Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Discover][Alerting] Implement editing of data view, query, filters #130462

Closed
wants to merge 10 commits into from

Conversation

dimaanj
Copy link
Contributor

@dimaanj dimaanj commented Apr 18, 2022

Summary

DataView

Checklist

Delete any items that are not applicable to this PR.

@dimaanj dimaanj added Feature:Discover Discover Application release_note:feature Makes this part of the condensed release notes Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. v8.3.0 labels Apr 18, 2022
@dimaanj dimaanj self-assigned this Apr 18, 2022
Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a quick first feedback, I think for filters we might need to place an UI element like a plus button to add new filters because the current way this is done leads to confusion:
Bildschirmfoto 2022-04-20 um 23 12 51 So clicking on Filters wouldn't trigger adding a new one, there would be a dedicated UI element for that

@@ -1,3 +1,7 @@
html {
overflow: hidden;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead, is it possible to set the suggestionSize on the component to s?

I see a prop here:

I am not sure where/how to pass in this prop, so (as a very quick-and-dirty test) I hardcoded this value to s and get this result which I suspect addresses the original issue:

Screen Shot 2022-04-20 at 4 30 28 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's possible, already applied.

@@ -364,7 +364,7 @@ class SearchBarUI extends Component<SearchBarProps, State> {
isLoading={this.props.isLoading}
fillSubmitButton={this.props.fillSubmitButton || false}
prepend={
this.props.showFilterBar && this.state.query
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was removed to be able to show saved query management, without filter bar. Filter bar added manually, omitting some features. Would it be safe change?

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me using the old layout would work, question is if we need to adapt the layout here @andreadelrio ?
Bildschirmfoto 2022-04-25 um 12 27 33
One thing to also keep in mind, we might soon be able to use the unified search bar here:
#128401

one think that didn't work, I got an endless spinner when trying to edit the alert rule in Stack management
Bildschirmfoto 2022-04-25 um 12 29 26

@ryankeairns
Copy link
Contributor

I may be getting ahead as this PR is in draft, but I'll point these out as I noticed them while testing the flyout:

Should we prompt to close/save the flyout when they click back in the main KQL bar
Otherwise, you see this overlapping of elements:

Screen Shot 2022-04-25 at 8 10 12 AM

The previous search string is quite sticky
If I remove it from the main page search bar, it still remains in the flyout even after close/re-open/refresh.
Screen Shot 2022-04-25 at 8 13 24 AM

@kertal
Copy link
Member

kertal commented May 2, 2022

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

kibana-ci commented May 2, 2022

💔 Build Failed

Failed CI Steps

Test Failures

  • [job] [logs] Default CI Group #6 / Discover alerting Search source Alert should display warning about updated alert rule
  • [job] [logs] Default CI Group #6 / Discover alerting Search source Alert should display warning about updated alert rule
  • [job] [logs] Jest Tests #5 / SearchSourceAlertTypeExpression should render error prompt
  • [job] [logs] Jest Tests #5 / SearchSourceAlertTypeExpression should render loading prompt
  • [job] [logs] Jest Tests #5 / SearchSourceAlertTypeExpression should render SearchSourceAlertTypeExpression with expected components

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
stackAlerts 125 128 +3
unifiedSearch 157 158 +1
total +4

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
unifiedSearch 75 77 +2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
discover 407.9KB 408.0KB +117.0B
stackAlerts 202.8KB 206.2KB +3.4KB
unifiedSearch 116.2KB 117.8KB +1.5KB
total +5.0KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
unifiedSearch 11 12 +1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
discover 40.3KB 40.3KB +30.0B
stackAlerts 12.6KB 13.0KB +468.0B
unifiedSearch 87.8KB 88.0KB +246.0B
total +744.0B
Unknown metric groups

API count

id before after diff
unifiedSearch 79 81 +2

async chunk count

id before after diff
unifiedSearch 9 10 +1

ESLint disabled line counts

id before after diff
unifiedSearch 15 16 +1

References to deprecated APIs

id before after diff
stackAlerts 24 39 +15

Total ESLint disabled count

id before after diff
unifiedSearch 15 16 +1

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @dmitriynj

@kertal
Copy link
Member

kertal commented May 5, 2022

Should we prompt to close/save the flyout when they click back in the main KQL bar
Otherwise, you see this overlapping of elements:

@ryankeairns
I think what we could do is, using the flyout to own focus and getting closed when clicked outside

Bildschirmfoto 2022-05-05 um 10 07 10

However, AFAIK we don't have currently access to the way it's being rendered, so it would need to be solved in a separate PR ... or being a feature request for our alerting efforts

The previous search string is quite sticky If I remove it from the main page search bar, it still remains in the flyout even after close/re-open/refresh.

Ok, if this is still the case this should be solved. every time the flyout is opened it should be reset to the current state of our search.

@dimaanj
Copy link
Contributor Author

dimaanj commented May 6, 2022

Closing current PR, since #131688 created.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Discover Discover Application release_note:feature Makes this part of the condensed release notes Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. v8.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants