Skip to content
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

redraw <chart type> with no changes is not a noop (svg mocks) #7347

Open
Tracked by #7346
marthacryan opened this issue Jan 25, 2025 · 0 comments
Open
Tracked by #7346

redraw <chart type> with no changes is not a noop (svg mocks) #7347

marthacryan opened this issue Jan 25, 2025 · 0 comments
Labels
bug something broken testing automated tests

Comments

@marthacryan
Copy link
Collaborator

marthacryan commented Jan 25, 2025

  • We traced it back to 2.24.0 (https://github.com/plotly/plotly.js/releases/tag/v2.24.0), which includes a group of PR's that correspond to the exact same figure types that are failing. For example, in the sunburst PR, there's a line that updates the trace marker color.
  • From @alexcjohnson:
    • In sunburst/style.js (which is part of the plotting pipeline) we have
    if(marker.pattern) {
        if(!marker.colors || !marker.pattern.shape) marker.color = cdi.color;
    } else {
        marker.color = cdi.color;
    }
    
    • ie we’re changing _fullData during plotting. Which is a big no no. So yeah that’s where the noop is being broken, we should avoid that, which I bet in this case means creating a new mock trace object to pass into Drawing.pointStyle(s, trace, gd, pt); rather than modifying this one. But of course since this has been around for a year and a half, this is not a release blocker.
    • Also I’ll note, the code in question is in styleOne, meaning that it’ll be called once for every segment of the sunburst… so if you do go with a mock trace object, put the object creation up in style (which only happens once per trace) instead of in styleOne, and styleOne can keep reusing that same object.
      If it helps, you’re free to attach new things to the trace object during plotting as long as they start with _.
    • I’m thinking about things like here where we mock an axis in order to reuse logic from regular axis handling in 3D axes… or here where we mock the entire figure in the course of making a new shrunken version of the figure for rangesliders
  • Update after looking into it a bit: The code referenced above is now is fill_one.js. Added in this commit
@marthacryan marthacryan changed the title can redraw <chart type> with no changes as a noop (svg mocks) redraw <chart type> with no changes is not a noop (svg mocks) Jan 25, 2025
@gvwilson gvwilson added bug something broken testing automated tests labels Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken testing automated tests
Projects
None yet
Development

No branches or pull requests

2 participants