-
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
style: Wrap chart titles again #11602
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11602 +/- ##
==========================================
- Coverage 66.57% 61.45% -5.13%
==========================================
Files 876 420 -456
Lines 42179 26193 -15986
Branches 3954 0 -3954
==========================================
- Hits 28082 16096 -11986
+ Misses 13994 10097 -3897
+ Partials 103 0 -103
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Could be a dashboard/slice level config, too! |
LGTM, but again, from a product stand point, i don't think wrapping long chart title in dashboard is the best visual solution nor the best user behavior we want to encourage.if that makes sense to the users, we are alright to bring the long title back. |
Another idea is to use dynamic font size based on text length. We already do that for some visualizations and there is a utility function in Personally I feel it makes more sense to wrap the title instead of truncating it because users put in long titles for a reason---if they really feel it looks bad, they would choose a shorter title already. |
Our original design/implementation is to have tooltip showed while user hover truncated title. so that user can view a cleaner dashboard without seeing overloaded information yet being able to access information as they need. My understanding is user have to put in long time is because
|
The full title is shown on hover right now via a |
Force user to hover chart one by one? |
@graceguo-supercat this PR gives you exactly what you're asking for... is there a reason not to approve it? |
haha No. Sorry i just forgot :) Thanks for make it happen!! |
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
Junlin's outline of the finer points of this design challenge is very insightful. We should look at better ways to address the true underlying user needs driving this discussion. But for now I think removing title truncation makes sense. |
Fixes #11581 |
Obviously this is one contentious line of CSS! Some of us like shorter chart headers... some of us don't. I can see a few ways to tackle things like it in the future:
The important thing (to me anyway) is that Superset is going to have a lot of changes... we need to get better at: ... but again, much ado about just one line... merging! |
Before implementation, a design document which is clear, accurate, highlight all the changes, public accessible to open source community, will help us. |
It's quite a luxury and ideal to have that when we can, and we're committed to get better at this. But realistically won't always be possible depending on the work at hand. Personally what I think is important is the ability to quickly get to a state that a majority is happy with, and sometimes it'll require some back and forth. It's hard to know ahead of time which areas might be contentious too. |
* style: letting chart titles wrap again * deleting comment (cherry picked from commit 32e52e9)
* style: letting chart titles wrap again * deleting comment
SUMMARY
This PR re-enables line-wrapping. Truncation might be re-enabled as a feature flag or something for those who prefer it, in a subsequent PR.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
After:
TEST PLAN
ADDITIONAL INFORMATION