Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 VisLayer error toasts when rendering vis #3649
Add VisLayer error toasts when rendering vis #3649
Changes from all commits
b3982af
6b593f3
72c4d64
2a8ac75
593b73a
739ddaa
485a8e0
7fef246
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
style nit, no need to change: this is basically a
join
operation on an array of strings - we could refactor this message composition so that each of theforEach
loops are just mapping operations that return arrays of strings and then join them together to form the final message rather than just concatenating everything together as we go.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.
why is the toast empty? Can you add a screenshot of the toast to the PR? it will be helpful to see what this looks like. Also you might want to add an id to the toast if you want to prevent he toast from being displayed again should the same expression run again before the toast is dismissed (But be careful to keep the ID specific to he vis since the id is global to the dashboard)
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.
I am also a little confused why the toast is empty here. In this case there was no vis layer errors, why is there a default message still with "error loading data on the chart"
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.
The reason for the toast message being empty here is because it is reserved for showing up in the modal details (the danger callout text). Added a screenshot of the toast to description.
I'll add an ID to the toast after rebasing and pulling in the upstream changes that include the ID as an option.
Note this is all happening inside an
if (err !== undefined)
block. This is only true when one or more VisLayers had an error. I'll add a comment in there too.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.
Update: after checking #3752 it doesn't include updating
ErrorToastOptions
which is needed for adding an ID toaddError
fn used for displaying error toasts. I will leave this empty for now and we can add later if that is decided to be added.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.
I've added the ID so it can be handled for
addError
scenarios. See commit 739ddaaConfirmed that the toast only shows up once on multiple re-renders.
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.
Is the ID here coming from the plugin resource ID?
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.
Using the visualize_embeddable ID such that it's unique within the dashboard. This enforces that there will only be one toast per vis on a dashboard.
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.
Did you do any testing what the toast would look like or have seen before if we have dozens of detector per the viz and they all have some error, will the toast be scrollable at some point?
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.
I haven't individually tested that but yes, it will be scrollable. It is designed to hold very long text (error stack traces)
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.
Sorry to reopen the thread, I'm just trying to understand whether our toast interface needs to be improved. The mocks make sense to me, where we have only a title in the toast. But why do we need to pass a whitespace character as
toastMessage
? It seems like we should be able to omit that property altogether, or else pass undefined, null, or at least an empty string. Do we need to open an issue to improve theaddError
method?