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

[Notifications] Error Toasts display top-level error type and reason in addition to caused_by #81965

Closed
wants to merge 3 commits into from

Conversation

rylnd
Copy link
Contributor

@rylnd rylnd commented Oct 28, 2020

Summary

Addresses #81857. In general, this additional info should provide more context to the user and not be redundant with the information in caused_by, but I'd appreciate if folks could help me try out some error cases, here.

Example toast when previewing an invalid EQL query (via the EQL search strategy):
Detections_-_Kibana

Where the caused_by.reason is in fact null:
Detections_-_Kibana

Checklist

For maintainers

The top-level error often contains more contextual information, and is
included more often than the caused_by error; this leads to more
information for the user.
@rylnd rylnd requested a review from kertal October 28, 2020 23:10
@rylnd rylnd requested a review from a team as a code owner October 28, 2020 23:10
@rylnd rylnd self-assigned this Oct 28, 2020
@rylnd rylnd added the release_note:skip Skip the PR/issue when compiling release notes label Oct 28, 2020
@timroes timroes added v7.11.0 and removed v7.11 labels Oct 29, 2020
@rylnd
Copy link
Contributor Author

rylnd commented Oct 30, 2020

@elasticmachine merge upstream

@kertal kertal requested a review from lizozom November 2, 2020 07:00
@kertal
Copy link
Member

kertal commented Nov 2, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

page load bundle size

id before after diff
core 544.6KB 543.9KB -697.0B

History

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

Comment on lines +71 to +73
if (error.caused_by != null) {
text += `${error.caused_by.type}\n`;
text += `${error.caused_by.reason}\n`;
Copy link
Contributor

@pgayvallet pgayvallet Nov 2, 2020

Choose a reason for hiding this comment

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

I see #70404 and #81857, and I'm starting to feel like this tries to solve a need specific to a single app, as opposite that something that all the toasts.addError consumers may need. Am I assuming incorrectly here? If not, maybe using toasts.add with a custom component as text would be more suitable than adding specific logic in the 'generic' ErrorToast component?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 I see now that the fields I'm proposing were displayed in the title for #70404. We could do that, or us a custom component as you propose.

I'm happy to close this and implement an app-specific solution; if others need this functionality we can always reopen.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to be clear: I'm fine with the implementation if this is something that is going to be widely used across multiple apps. I would just like to avoid app-specific code landing into core.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pgayvallet agreed! I'm happy to promote this to core when the need arises.

@rylnd
Copy link
Contributor Author

rylnd commented Nov 2, 2020

Closing as per #81965 (comment)

@rylnd rylnd closed this Nov 2, 2020
@rylnd rylnd deleted the more_error_toast_info branch November 2, 2020 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes v7.10.1 v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants