-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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): Re-enable Explore re-render on Save As #20704
fix(explore): Re-enable Explore re-render on Save As #20704
Conversation
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.
We can't have Explore actions inside the datasource module because datasources are a shared resource. Let's sync on this.
f9c8f71
to
95dc1d7
Compare
Codecov Report
@@ Coverage Diff @@
## master #20704 +/- ##
==========================================
- Coverage 66.83% 66.83% -0.01%
==========================================
Files 1750 1750
Lines 65894 65895 +1
Branches 7017 7018 +1
==========================================
- Hits 44041 44039 -2
- Misses 20067 20069 +2
- Partials 1786 1787 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
Approving this as a hot-fix. We should remove Explore specific state from the datasources reducer.
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.
We can't remove initDatasources
. Can you restore that line and check if the bug continues to be fixed?
Closing in favor of #20796. |
SUMMARY
The combination of #20498 and #20645 caused a regression in Explore logic such that when a chart is saved as a new chart, the chart wouldn't render once the modal was closed without clicking the "Update Chart" button. This broke a Cypress test,
explore/link.test.ts
that's currently failing on master, but which wasn't picked up by CI before merge because CI on #20645 was run before #20498 had been merged and the two PRs didn't have any conflicts that would prompt GH to require a rebase. This PR reverts a little bit of #20645, making theHYDRATE_EXPLORE
Redux action once again update thedatasources
store key in the same step instead of in a different step. I don't really understand why this change fixes the issue, but it seems like the Explore components only refresh as expected ifdatasources
is updated as part of theHYDRATE_EXPLORE
action.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
Screen.Recording.2022-07-13.at.7.25.46.PM.mov
After:
Screen.Recording.2022-07-13.at.7.23.58.PM.mov
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION