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: Monospacing errors in dashboards & charts #18796

Merged
merged 2 commits into from
Feb 25, 2022

Conversation

codemaster08240328
Copy link
Contributor

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

Before
image

image

After
image

image

TESTING INSTRUCTIONS

@apache apache deleted a comment from github-actions bot Feb 17, 2022
@apache apache deleted a comment from github-actions bot Feb 17, 2022
@codecov
Copy link

codecov bot commented Feb 17, 2022

Codecov Report

Merging #18796 (683c265) into master (2d67d2f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 683c265 differs from pull request most recent head 93b3f7d. Consider uploading reports for the commit 93b3f7d to get more accurate results

Impacted file tree graph

@@           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           
Flag Coverage Δ
javascript 51.26% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
superset-frontend/src/chart/Chart.jsx 53.44% <100.00%> (+1.66%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d67d2f...93b3f7d. Read the comment docs.

@rusackas
Copy link
Member

/testenv up

@github-actions
Copy link
Contributor

@rusackas Ephemeral environment spinning up at http://54.185.22.55:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@rusackas
Copy link
Member

This has a side effect of monospacing ANY error in this dashboard/explore context. See screenshot below of one example.

image

@kasiazjc any objections to this? I think the pros outweigh the cons here.

@kasiazjc
Copy link
Contributor

@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 🤔

Copy link
Member

@villebro villebro left a 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`
Copy link
Member

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

@rusackas rusackas merged commit 4923256 into apache:master Feb 25, 2022
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

villebro pushed a commit that referenced this pull request Apr 3, 2022
* fix: Monospacing errors in dashboards & charts

* removed unnecessary styling

(cherry picked from commit 4923256)
@mistercrunch mistercrunch added 🍒 1.5.0 🍒 1.5.1 🍒 1.5.2 🍒 1.5.3 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels lts-v1 size/S 🍒 1.5.0 🍒 1.5.1 🍒 1.5.2 🍒 1.5.3 🚢 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants