Skip to content

fix(revert 27883): Excess padding in horizontal Bar charts#29345

Merged
michael-s-molina merged 2 commits into
apache:masterfrom
michael-s-molina:fix-horizontal-bar-padding
Jun 25, 2024
Merged

fix(revert 27883): Excess padding in horizontal Bar charts#29345
michael-s-molina merged 2 commits into
apache:masterfrom
michael-s-molina:fix-horizontal-bar-padding

Conversation

@michael-s-molina
Copy link
Copy Markdown
Member

@michael-s-molina michael-s-molina commented Jun 24, 2024

SUMMARY

This PR reverts #27883 which incorrectly calculated the chart's padding and introduced #29342.

There's a bug with yAxis.inverse in ECharts and we don't know when a fix will be provided. To fix this issue, this PR partially reverts #23273 which introduced said configuration. I'm purposefully not adding a migration to this PR, as #23273 did, so it can be cherry-picked as it's affecting all 4.0.x versions. This will change the default y-axis order of horizontal bar charts. @yousoph

An alternative solution, would be to fix the ECharts bug. @villebro

Fixes #29342

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screenshot 2024-06-24 at 11 16 37 Screenshot 2024-06-24 at 11 16 02 Screenshot 2024-06-24 at 10 51 52 Screenshot 2024-06-24 at 10 51 27

TESTING INSTRUCTIONS

1 - Create an horizontal bar chart using a time column in the x-axis
2 - Check that the chart is displayed correctly.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@michael-s-molina michael-s-molina requested a review from sfirke June 24, 2024 14:49
const xAxisOffset = addXAxisTitleOffset ? Number(xAxisTitleMargin) || 0 : 0;
const showLegendTopOffset =
isHorizontal && showLegend && legendOrientation === LegendOrientation.Top
? 100
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to revert the whole thing, or should this 100px just be a more reasonable number?

Copy link
Copy Markdown
Member Author

@michael-s-molina michael-s-molina Jun 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You cannot set a fixed padding because it will depend on the size of each bar generated by ECharts, which will also vary depending on the number of bars. This calculation should be done automatically by ECharts.

@rusackas rusackas changed the title fix: Excess padding in horizontal Bar charts fix(revert 27883): Excess padding in horizontal Bar charts Jun 24, 2024
Copy link
Copy Markdown
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving to unblock, but wondering if the fix-forward is simply changing a number.

@michael-s-molina
Copy link
Copy Markdown
Member Author

Approving to unblock, but wondering if the fix-forward is simply changing a number.

Before merging this, I still want @villebro's input. Maybe we can fix the bug in ECharts and avoid reverting #23273.

@michael-s-molina michael-s-molina added the hold! On hold label Jun 24, 2024
Copy link
Copy Markdown
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.

LGTM

@michael-s-molina michael-s-molina removed the hold! On hold label Jun 25, 2024
@michael-s-molina michael-s-molina merged commit 708afb7 into apache:master Jun 25, 2024
@michael-s-molina michael-s-molina added the v4.0 Label added by the release manager to track PRs to be included in the 4.0 branch label Jun 25, 2024
michael-s-molina added a commit that referenced this pull request Jun 26, 2024
@mistercrunch mistercrunch added 🍒 4.0.2 Cherry-picked to 4.0.2 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels labels Jul 24, 2024
@mistercrunch mistercrunch added the 🚢 4.1.0 First shipped in 4.1.0 label Nov 27, 2024
ratuldawar11 pushed a commit to grofers/superset that referenced this pull request Apr 14, 2026
* fix(permalink): adding anchor to dashboard permalink generation (apache#28744)

* fix: filters not updating with force update when caching is enabled (apache#29291)

(cherry picked from commit 527f1d2)

* fix(sqllab): invalid empty state on switch tab (apache#29278)

* fix(metastore-cache): prune before add (apache#29301)

(cherry picked from commit 172ddb4)

* fix: Remove recursive repr call (apache#29314)

(cherry picked from commit 9444c6b)

* fix: Cannot delete empty column inside a tab using the dashboard editor (apache#29346)

(cherry picked from commit ee52277)

* fix(explore): restored hidden field values has discarded (apache#29349)

(cherry picked from commit 160cece)

* chore: Rename Totals to Summary in table chart (apache#29360)

* fix(revert 27883): Excess padding in horizontal Bar charts (apache#29345)

(cherry picked from commit 708afb7)

* fix(explore): don't respect y-axis formatting (apache#29367)

* fix: adds the ability to disallow SQL functions per engine (apache#28639)

* chore: Adds 4.0.2 RC2 data to CHANGELOG.md

* fixes

* frontend fixes

* fix: cache api

---------

Co-authored-by: Jack <41238731+fisjac@users.noreply.github.com>
Co-authored-by: ka-weihe <k@weihe.dk>
Co-authored-by: JUST.in DO IT <justin.park@airbnb.com>
Co-authored-by: Ville Brofeldt <33317356+villebro@users.noreply.github.com>
Co-authored-by: Jessie R <j@scjr.me>
Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>
Co-authored-by: Daniel Vaz Gaspar <danielvazgaspar@gmail.com>
Co-authored-by: Michael S. Molina <michael.s.molina@gmail.com>
qfcwell pushed a commit to qfcwell/superset that referenced this pull request May 12, 2026
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 plugins size/S v4.0 Label added by the release manager to track PRs to be included in the 4.0 branch viz:charts:bar Related to the Bar chart viz:charts:echarts Related to Echarts 🍒 4.0.2 Cherry-picked to 4.0.2 🚢 4.1.0 First shipped in 4.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Excess Padding in Horizontal Bar Charts When Legend is Added

4 participants