-
Notifications
You must be signed in to change notification settings - Fork 16.1k
fix: Update spacing on echart legends #34018
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
Conversation
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 completed my review and didn't find any issues.
Files scanned
| File Path | Reviewed |
|---|---|
| superset-frontend/packages/superset-ui-core/src/utils/html.tsx | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
|
Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment
|
|
@sadpandajoe Processing your ephemeral environment request here. Action: up. More information on how to use or configure ephemeral environments |
|
@sadpandajoe Ephemeral environment spinning up at http://35.161.120.126:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup. |
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 completed my review and didn't find any issues.
Files scanned
| File Path | Reviewed |
|---|---|
| superset-frontend/packages/superset-ui-core/src/utils/html.tsx | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
| table: ['width', 'border', 'align', 'valign', 'style'], | ||
| tr: ['rowspan', 'align', 'valign', 'style'], | ||
| td: ['width', 'rowspan', 'colspan', 'align', 'valign', 'style'], | ||
| th: ['width', 'rowspan', 'colspan', 'align', 'valign', 'style'], | ||
| tbody: ['align', 'valign', 'style'], | ||
| thead: ['align', 'valign', 'style'], | ||
| tfoot: ['align', 'valign', 'style'], |
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.
Not sure about allowing style tags here, can't we change the styles directly?
https://cheatsheetseries.owasp.org/cheatsheets/XSS_Filter_Evasion_Cheat_Sheet.html#style-attribute-that-breaks-up-an-expression
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 think we tried to add the styles here: https://github.com/apache/superset/blob/master/superset-frontend/packages/superset-ui-core/src/utils/tooltip.ts, but when we sanitized it, it then broke the spacing and formatting. If we think what we did with theming could change how we redo tooltips, we should probably implement it the correct way.
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.
Yeah, those styles do nothing. I don't think theming can change how we handle tooltips, maybe setting classname might be the better way to do this?
betodealmeida
left a comment
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.
LGTM
(cherry picked from commit cb6342f)
SUMMARY
The legend looks a bit more squished on master now than it was before with 4.X and 5.X. This PR will get it back to that size.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:

After:

TESTING INSTRUCTIONS
ADDITIONAL INFORMATION