-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Add async search notification #60706
Add async search notification #60706
Conversation
Pinging @elastic/kibana-app-arch (Team:AppArch) |
3567441
to
2e35fde
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a couple of comments below, maybe we can sync on these things together.
src/plugins/data/public/ui/query_string_input/query_bar_top_row.tsx
Outdated
Show resolved
Hide resolved
src/plugins/data/public/ui/query_string_input/query_bar_top_row.tsx
Outdated
Show resolved
Hide resolved
Here are a few things I'm noticing (and questions I have):
The behavior of icons on notifications is the same for all types of notifications (they all have icons). And this won't happen if you didn't slow down the network intentionally.
@AlonaNadler should I remove the cancel button altogether?
Fixed. Now it will stay on screen until search completes.
Updated to the copy from the doc. You can see updated screenshots on the issue.
The timer starts from the moment the search request is being sent.
Yes, that's the behavior - cancel the notification and show an error. |
I read @mdefazio suggestions, not sure I understand why to remove the cancel button |
@AlonaNadler At this stage, doing any action on the dashboard/page would in effect cancel the request. We also try and keep toasts to a single CTA. I know things will change in 7.8, so I'm not saying we don't need it then. But it feels a bit unnecessary now. |
@lukasolson if we decide to remove the cancel button, then I'd also suggest changing the button style to hollow. If we keep it though, then no other changes on my end. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
* notifications ui * increase timeout to 10s * trigger notification from search interceptor * added an enhanced interceptor * added an enhanced interceptor * docs * docs * fix ts * Fix jest tests for interceptor * update docs * docs * Fix handling syntax error in discover * docs and translations * fix scripted fields err Co-authored-by: Lukas Olson <[email protected]>
* notifications ui * increase timeout to 10s * trigger notification from search interceptor * added an enhanced interceptor * added an enhanced interceptor * docs * docs * fix ts * Fix jest tests for interceptor * update docs * docs * Fix handling syntax error in discover * docs and translations * fix scripted fields err Co-authored-by: Lukas Olson <[email protected]> Co-authored-by: Lukas Olson <[email protected]>
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
Summary
Depends on #60574
Resolves #24227.
Resolves #46370.
Add async search notification.
In OSS:
In Basic:
Dev Docs
The async search feature allows users with a Basic license and above, execute long queries. By default, query execution is limited to 30 second time frame. Now Discover, Visualize and most importantly Dashboard applications, will present the user an option to run a query beyond timeout.
Checklist
Delete any items that are not applicable to this PR.
For maintainers