Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

fix: charts rerender with zero-height during tab switch #703

Merged
merged 1 commit into from
Jul 27, 2020

Conversation

ktmud
Copy link
Contributor

@ktmud ktmud commented Jul 25, 2020

🐛 Bug Fix

Some charts rely on the dimension of mounted chart elements (node.clientWidth and node.clientHeight) to adjust layout, but both values are zero when Bootstrap tabs are still animating. The unfortunate timing leads to some charts unable to re-render when switching tabs after applying filters (see apache/superset#10349 ).

Ideally we should fix the Tabs component and only re-render charts when animation finishes but I was not able to find a clean way to do that, so fixing the known broken charts instead.

Test Plan

Manual testing

@ktmud ktmud requested a review from a team as a code owner July 25, 2020 06:28
@vercel
Copy link

vercel bot commented Jul 25, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/superset/superset-ui/kc981xmfi
✅ Preview: https://superset-ui-git-fork-ktmud-bugfix-tab-chart-rerender.superset.vercel.app

@codecov
Copy link

codecov bot commented Jul 25, 2020

Codecov Report

Merging #703 into master will increase coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #703   +/-   ##
=======================================
  Coverage   24.21%   24.22%           
=======================================
  Files         338      338           
  Lines        7607     7608    +1     
  Branches      924      925    +1     
=======================================
+ Hits         1842     1843    +1     
  Misses       5692     5692           
  Partials       73       73           
Impacted Files Coverage Δ
...gins/legacy-plugin-chart-world-map/src/WorldMap.js 0.00% <0.00%> (ø)
...egacy-plugin-chart-world-map/src/transformProps.js 0.00% <0.00%> (ø)
...ugin-chart-table/src/DataTable/hooks/useSticky.tsx 3.57% <0.00%> (-0.09%) ⬇️
packages/superset-ui-query/src/extractExtras.ts 100.00% <0.00%> (ø)
packages/superset-ui-query/src/processFilters.ts 100.00% <0.00%> (ø)
...kages/superset-ui-query/src/types/QueryFormData.ts 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2923973...c3789d4. Read the comment docs.

@villebro
Copy link
Contributor

Nice @ktmud 👍 Do you know if this problem specific to Bootstrap, and would the corresponding AntD Tab component work correctly without a similar fix?

@ktmud
Copy link
Contributor Author

ktmud commented Jul 25, 2020

@villebro I haven't tested. It depends on how they implemented the transition animation.

Copy link
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.

LGTM! Thanks for catching/fixing this!

@villebro
Copy link
Contributor

@ktmud feel free to merge and deploy at will so we can bump the package in incubator-superset

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants