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

fix(ui): add error messaging for cells in dashdboard view #17754

Merged
merged 3 commits into from
Apr 15, 2020

Conversation

TCL735
Copy link
Contributor

@TCL735 TCL735 commented Apr 15, 2020

Closes #17594

Add error messaging for cells in the Dashboard View.

Example: when one cell has error
Screen Shot 2020-04-15 at 11 25 11 AM

Example: when all cells have error
Screen Shot 2020-04-15 at 11 25 39 AM

@TCL735 TCL735 requested review from alexpaxton, a team, ebb-tide and drdelambre and removed request for a team April 15, 2020 17:54
@alexpaxton
Copy link
Contributor

image

Overall looks good, there are these massive gaps between the error box and the cell header. Not sure where that gap is coming from, but I am happy to pair on removing it if that helps

Copy link
Contributor

@alexpaxton alexpaxton left a comment

Choose a reason for hiding this comment

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

Gaps!

@@ -13,7 +13,7 @@

See https://github.com/influxdata/flux/blob/master/docs/SPEC.md#errors.
*/
export const checkQueryResult = (file: string): void => {
export const checkQueryResult = (file: string = ''): void => {
Copy link
Contributor

Choose a reason for hiding this comment

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

what does this do?

Copy link
Contributor Author

@TCL735 TCL735 Apr 15, 2020

Choose a reason for hiding this comment

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

checkQueryResult is called here

checkQueryResult(result.csv)

At runtime, result.csv may be undefined (not a string) when there is an error. The code immediately above the call to checkQueryResult throws a runtime error which will hide the fact that checkQueryResult will throw its own error when result.csv is not a string.

By adding a default value to the parameter we prevent checkQueryResult from throwing its own error (which would be visible in the console should the throws above it are ever removed).

Copy link
Contributor Author

@TCL735 TCL735 Apr 15, 2020

Choose a reason for hiding this comment

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

Specifically, the error messaging will become checkQueryResult's thrown error rather than the actual error. And we want the error message to be useful to the user (rather than the dev). The user won't care or know what to do with "Cannot read property 'slice' of undefined"

Screen Shot 2020-04-15 at 11 43 58 AM

@TCL735
Copy link
Contributor Author

TCL735 commented Apr 15, 2020

@alexpaxton Gaps are removed. I have updated the screenshots.

@TCL735 TCL735 requested a review from alexpaxton April 15, 2020 18:50
@TCL735 TCL735 merged commit 0ec94e9 into master Apr 15, 2020
@TCL735 TCL735 deleted the fix_17594_dashboard_cell_error_msg branch April 15, 2020 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memory allocation errors no longer show on dashboards
3 participants