-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Security Solution] Rule installation/upgrade error handling fixes #159823
Conversation
Pinging @elastic/security-solution (Team: SecuritySolution) |
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
} | ||
return toastMessage; | ||
return toastMessages.join(' '); |
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.
nit: Little curious about the toast UX here since skipped
+ failed
are included in the onSuccess
toast (so long as the explicit onError
hasn't occurred.)
Splitting out into separate toasts feels like toast spam, and doesn't look like we can add \n
's to toast titles, so it ends up looking something like this:
Maybe a toast w/ title + description would work here if toastMessages.length > 1
, and then have each message on a new line in the description? Could also color a warning
toast if skipped
or failed
were > 0?
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.
Maybe a toast w/ title + description would work here if
toastMessages.length > 1
, and then have each message on a new line in the description? Could also color awarning
toast ifskipped
orfailed
were > 0?
I like the suggestion. Thank you, @spong 👍 I'm going to implement it in a follow-up
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.
Code changes LGTM! 👍 Left one nit/thought on how we compose the resulting toast, but no changes necessary. Thanks @xcrzx!
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Async chunks
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
To update your PR or re-run it, just comment with: cc @xcrzx |
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.
Thanks for these fixes! 👍
Hope it's fine by you, I'm getting this merged to prevent conflicts with the ML Jobs popover work I'm doing
Related to: #158450
Summary
Various fixes to the rule installation and upgrade workflows: