-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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: Monospacing errors in dashboards & charts #18796
fix: Monospacing errors in dashboards & charts #18796
Conversation
Codecov Report
@@ Coverage Diff @@
## master #18796 +/- ##
=======================================
Coverage 66.32% 66.32%
=======================================
Files 1620 1620
Lines 63087 63089 +2
Branches 6372 6372
=======================================
+ Hits 41840 41842 +2
Misses 19590 19590
Partials 1657 1657
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
/testenv up |
@rusackas Ephemeral environment spinning up at http://54.185.22.55:8080. Credentials are |
This has a side effect of monospacing ANY error in this dashboard/explore context. See screenshot below of one example. @kasiazjc any objections to this? I think the pros outweigh the cons here. |
@rusackas does error always contain sql/code? If yes, I think that pros outweigh the cons if we cannot separate those. If no - this could create more confusion 🤔 |
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 fixing this @codemaster08240328! This has been an annoyance for a long time, especially on BigQuery where error messages have been very difficult to decipher due to the type face being variable width.
@kasiazjc @rusackas due to the varied exception types we get from analytical databases it will often be difficult to distinguish between code-like exceptions that need monospacing and more general exceptions that don't. We can potentially add more fine-grained logic to the backend that tries to identify which messages need monospacing and which don't, but until we do I suggest merging this as-is, as I feel the problem this fixes is more severe than the side-effect it introduces.
@@ -113,6 +113,14 @@ const RefreshOverlayWrapper = styled.div` | |||
justify-content: center; | |||
`; | |||
|
|||
const MonospaceDiv = styled.div` |
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: I would perhaps consider naming this MonospaceMessage
Ephemeral environment shutdown and build artifacts deleted. |
* fix: Monospacing errors in dashboards & charts * removed unnecessary styling (cherry picked from commit 4923256)
SUMMARY
On Charts and Dashboards, if the user has an error, the error message should be displayed in a monospace font so some of the formatting / details is not lost.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS