fix(echarts): only reserve left grid padding when a Y-axis title exists - #43064
Closed
s1ny1998 wants to merge 1 commit into
Closed
fix(echarts): only reserve left grid padding when a Y-axis title exists#43064s1ny1998 wants to merge 1 commit into
s1ny1998 wants to merge 1 commit into
Conversation
getPadding() added y_axis_title_margin to the left grid offset whenever y_axis_title_position was 'Left', without checking that a Y-axis title was actually rendered. Since the control defaults are position 'Left' and margin 50, every timeseries chart with no Y-axis title reserved 70px on the left. With grid.containLabel enabled that space sits outside the tick labels, so it is pure dead space. It is unnoticeable on a wide desktop chart but consumes roughly a third of the plot area on a phone. Gate the left margin on the existing addYAxisTitleOffset flag, which callers already compute as `!!yAxisTitle && margin !== 0`, and have Gantt pass that flag instead of a hardcoded false so it keeps its padding when a title is present.
Contributor
|
Bito Automatic Review Skipped - Branch Excluded |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## mobile-dashboard-support #43064 +/- ##
=========================================================
Coverage 65.80% 65.80%
=========================================================
Files 2845 2845
Lines 162333 162335 +2
Branches 37256 37258 +2
=========================================================
+ Hits 106822 106824 +2
Misses 53446 53446
Partials 2065 2065
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Author
|
Waiting on @rusackas for approval. Hence marked as draft. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
On mobile, timeseries charts render with a large blank strip on the left: the plot area starts roughly a third of the way into the card, well outside the tick labels.
The cause is in
getPadding()(plugins/plugin-chart-echarts/src/Timeseries/transformers.ts):The Y-axis title margin is added whenever the title position is
'Left', without checking that a title is actually rendered. The control defaults (sections/chartTitle.tsx) are position'Left'and margin50, so every chart with an emptyy_axis_title— the default — reserves20 + 50 = 70pxon the left. Becausegrid.containLabelis enabled, that reservation sits outside the axis labels, so it is pure dead space.The guard already exists: callers compute
addYAxisTitleOffset = !!yAxisTitle && convertInteger(yAxisTitleMargin) !== 0, and the'Top'position branch consumes it. Only the'Left'branch ignored it.Changes:
addYAxisTitleOffset. Charts with a Y-axis title keep exactly the spacing they have today; charts without one reclaim 50px of plot area.Gantt/transformProps.tshardcodedfalsefor that flag at bothgetPadding()call sites while relying on the'Left'branch for its left padding, so it now computes the flag properly and passes it to both. Without this, Gantt charts that do have a title would lose their title padding.getPadding()unit test for the no-title case.Affected chart families: Timeseries (line/bar/area/scatter/smooth), Mixed Timeseries, Box Plot, Gantt. Bubble already passed
trueunconditionally and is unchanged.This is negligible on wide desktop charts and very visible on narrow ones, which is why it surfaced in mobile consumption mode.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before: plot area begins ~95px into a ~320px-wide mobile chart (70px reserved margin + tick-label width), leaving the series squeezed into the right portion of the card.
After: only
gridOffsetLeft(20px) plus the tick-label width is reserved, and the series fills the card.TESTING INSTRUCTIONS
Leftand a non-zero margin. The reserved space returns and the rotated title renders inside it, unchanged from before.Topto confirm top spacing is untouched.Unit tests:
56 suites / 655 tests pass, including the new
getPaddingcase.ADDITIONAL INFORMATION