-
Notifications
You must be signed in to change notification settings - Fork 28
Do not mutate plot props when zooming #5066
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
Conversation
) : ( | ||
<VegaLite {...plotProps} onNewView={onNewView} /> | ||
))} | ||
{isTemplatePlot ? ( |
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.
Similar blocks of code found in 2 locations. Consider refactoring.
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.
Using the full Vega API for signals and views (https://vega.github.io/vega/docs/api/view/#signals) to make things simpler
Code Climate has analyzed commit ec37342 and detected 3 issues on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 100.0% (85% is the threshold). This pull request will bring the total coverage in the repository to 95.3% (0.0% change). View more on Code Climate. |
@@ -50,18 +50,13 @@ const PlotsContent = () => { | |||
return <EmptyState>Loading Plots...</EmptyState> |
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.
Not quite sure what is going on, but when I tested the smooth plots, the plots started behaving really weird:
Screen.Recording.2023-12-05.at.9.46.02.AM.mov
I doubled checked to make sure it wasn't happening on main
and main
is working correctly.
Dependency management between Studio/VS Code/DVC has turned out to be a bit more complicated than I thought it was going to be. To merge #4734 and roll out the rest of the plots PRs I am going to have to:
I'm going to pull the rug on this PR. Sorry. |
No worries. I think you already did the same thing or similar in your PR. For using the Vega API, it's easy to re-add later. |
Fixes the root cause of #4671
Demo:
Screen.Recording.2023-12-04.at.2.52.08.PM.mov
We previously fixed the bug when closing the section by working around the bug as we did in the past as well.
The bug was also seen when drag and dropping without changes and would have happened on every component restore.
The fix here is to not pass the props to the zoomed in props, but get them from there.
The changes inside
TemplateVegaLite
helped in finding out where the bug was and make things cleaner in general.