Skip to content

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

Closed
wants to merge 2 commits into from
Closed

Do not mutate plot props when zooming #5066

wants to merge 2 commits into from

Conversation

sroy3
Copy link
Contributor

@sroy3 sroy3 commented Dec 4, 2023

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.

@sroy3 sroy3 added the bug Something isn't working label Dec 4, 2023
@sroy3 sroy3 self-assigned this Dec 4, 2023
@sroy3 sroy3 marked this pull request as ready for review December 4, 2023 20:10
) : (
<VegaLite {...plotProps} onNewView={onNewView} />
))}
{isTemplatePlot ? (

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.

Copy link
Contributor Author

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

Copy link

Code Climate has analyzed commit ec37342 and detected 3 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 1
Duplication 2

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

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.

@mattseddon
Copy link
Contributor

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:

  1. make a dvclive PR with the version of dvc from iterative/dvc#9931
  2. test vscode-dvc
  3. test studio
  4. release dvc
  5. update dvclive PR from 1
  6. release dvclive
  7. update vscode-dvc PR
  8. merge vscode-dvc PR
  9. update studio PR
  10. merge studio PR

I'm going to pull the rug on this PR. Sorry.

@sroy3
Copy link
Contributor Author

sroy3 commented Dec 6, 2023

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.

@sroy3 sroy3 closed this Dec 6, 2023
@sroy3 sroy3 deleted the vega-smooth-signal branch December 6, 2023 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants