-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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(Explore): "Customize" tab rendering behavior #15841
Conversation
Codecov Report
@@ Coverage Diff @@
## master #15841 +/- ##
==========================================
- Coverage 77.13% 76.88% -0.25%
==========================================
Files 984 984
Lines 51706 51796 +90
Branches 6995 7032 +37
==========================================
- Hits 39882 39825 -57
- Misses 11600 11746 +146
- Partials 224 225 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
/testenv up |
@geido Ephemeral environment spinning up at http://52.11.127.107:8080. Credentials are |
@geido I'm not sure if this is related to this fix, but see my video Steps:
If we to save the state of collapse, then I think UI should also reflect that it is currently in collapse (down arrow) CleanShot.2021-07-22.at.10.13.58.mp4 |
@rosemarie-chiu thanks for spotting that one. From a first look, it appears to be a problem related to the original one. I will try to fix this issue as well with this PR. I'll ping you once I am done with this additional change. |
@junlincc @rosemarie-chiu this should be good to go for another round of testing. I have updated the description as well. |
/testenv up |
@geido Ephemeral environment spinning up at http://35.164.243.123:8080. Credentials are |
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.
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
Ephemeral environment shutdown and build artifacts deleted. |
* Remove getDerivedStateFromProps * Set loading
* Remove getDerivedStateFromProps * Set loading
* Remove getDerivedStateFromProps * Set loading
* Remove getDerivedStateFromProps * Set loading
SUMMARY
The bug was the consequence of different issues.
Issue with
getDerivedStateFromProps
:This PR removes the
getDerivedStateFromProps
introduced by PR #14493 that was causing the Collapse to not re-render when changing viz type as the new props were not propagated down to the render phase. The removal does not seem to affect the functionality for which that PR was made. @villebro your confirmation will be appreciated here.Issue with the Antdesign Collapse:
This problem seems related to a bug with Antdesign when multiple re-renders happen while the Collapse is opening up, causing the Collapse to render invisible (0 height). By forcing the rendering of a
Loading
component after changing the viz type/datasource, Antdesign is then able to properly render the Collapse.Fixes: #15829
BEFORE
126574252-c1fb3e64-712e-4dc9-abf0-fd4306b36b44.mov
AFTER
DEV.Number.of.mp4
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION