-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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: filters panel broken due to tabs scroll #30180
fix: filters panel broken due to tabs scroll #30180
Conversation
@michael-s-molina I also confirmed that this bug exists in 4.0 as well. |
// https://github.com/apache/superset/issues/30058 | ||
.echarts-tooltip[style*='visibility: hidden'] { | ||
display: none !important; | ||
} |
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.
Couple of options if you want to optimize further:
- we can probably narrow this from Superset global to being part of the ECharts plugin(s), perhaps using Emotion Globals.
- This might be a good excuse to open a PR on ECharts and fix it at the root ;)
Either way, I don't see this as a blocker for now. Things are lookin' good!
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.
Approach 2 is, of course, the fundamental solution, but I decided to leave it as a long-term solution considering the time gap involved in merging the patch and testing Superset compatibility with the corresponding version. (I plan to propose a related PR to echart later.)
Initially, I considered Approach 1, but I had concerns that adding a dependency on @emotion/react to the plugins' npm package might open the possibility for other custom plugins to apply custom global CSS. Therefore, I opted for this approach.
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... we can probably get fancy with how/where we do this, but this works well for today!
(cherry picked from commit be0a0ce)
SUMMARY
Fixes #30058
The EChart tooltip is hidden using
visibility: hidden
withopacity: 0
after it disappears, which causes tooltips generated from the previous tab to be recognized as scroll items when configuring the layout for the next tab, resulting in unnecessary scrollbars.This commit addresses the issue by using a CSS hack to change the visibility hidden tooltip to
display: none
, effectively removing it and resolving the scrollbar problem.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
before--css-hide-hack.mov
After:
after--css-hide-hack.mov
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION