Fixed issue gh-11327: Plotly chart reset axes not working after reset#11498
Conversation
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) |
sfc-gh-smohile
added
change:bugfix
frontend/lib/src/components/elements/PlotlyChart/PlotlyChart.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Vega-lite snapshots in Firefox seem to be extremely flaky since today.... not sure why :(
There was a problem hiding this comment.
Thank god! I've been scratching my head for 3 hours wondering how did my plotly changes break vega lite 😢
There was a problem hiding this comment.
yeah, my assumption is that there was some firefox patch update related playwright which causes some issues that prevents our theme changes to be applied to the charts. But I haven't looked deeper into this yet.
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
sfc-gh-lmasuch
left a comment
sfc-gh-lmasuch
left a comment
There was a problem hiding this comment.
LGTM 👍 but all the vega-lite snapshots need to be reverted to the original state.
There was a problem hiding this comment.
Just to confirm: the initialFigureSpec does not contain any height information we could use here? The 450px default is something that Plotly chooses in the frontend if no height is used?
There was a problem hiding this comment.
Yes, that is right. 450 is plotly's default height if no height is mentioned.
There was a problem hiding this comment.
Pull Request Overview
Fixes the bug where Plotly chart axes did not reset correctly after exiting fullscreen by adding a default height fallback and an end-to-end test.
- Added a fallback default height when
initialFigureSpec.layout.heightis undefined outside fullscreen. - Removed the dynamic
keyprop from thePlotcomponent. - Introduced a new Playwright E2E test to verify axis reset after fullscreen zoom.
Reviewed Changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| frontend/lib/src/components/elements/PlotlyChart/PlotlyChart.tsx | Adds default height fallback and removes the key prop on the Plot component |
| e2e_playwright/st_plotly_chart_test.py | Adds test_plotly_fullscreen_reset_axis to verify axes reset after zooming in/out of fullscreen |
Comments suppressed due to low confidence (1)
frontend/lib/src/components/elements/PlotlyChart/PlotlyChart.tsx:462
- Removing the
keyprop may prevent the Plot component from fully remounting when toggling fullscreen, which can lead to stale internal state. Consider restoring a dynamic key or explicitly resetting Plotly's layout/state.
key={isFullScreen ? "fullscreen" : "original"}
frontend/lib/src/components/elements/PlotlyChart/PlotlyChart.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Use nth(index) instead of nth(0) to ensure the exit-fullscreen button targets the same chart instance you entered fullscreen on.
| exit_fullscreen_button = app.locator('[data-title="Close fullscreen"]').nth(0) | |
| exit_fullscreen_button = app.locator('[data-title="Close fullscreen"]').nth(index) |
There was a problem hiding this comment.
suggestion: This snapshot and the webkit snapshot for this same test need to be reverted.
There was a problem hiding this comment.
suggestion: +1 to @sfc-gh-lmasuch's review comment: all of the snapshots in this directory need to be reverted.
Co-authored-by: graphite-app[bot] <96075541+graphite-app[bot]@users.noreply.github.com>
Co-authored-by: Copilot <[email protected]>
sfc-gh-smohile
force-pushed
the
fix/plotly-reset-axes-after-fullscreen
branch
from
b509193 to
7e7085a
Compare
Describe your changes
Fixed bug where Plotly Chart reset axes were not working after clicking full screen and going back.
GitHub Issue Link (if applicable)
#11327
Testing Plan
Explanation of why no additional tests are needed
Unit Tests (JS and/or Python)
E2E Tests: Added new E2E tests to verify that Plotly chart axes reset properly after entering and exiting fullscreen mode
Any manual testing needed?
No
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.