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

Hotfix: clean up visible false scatter trace #920

Merged
merged 3 commits into from
Sep 9, 2016

Conversation

etpinard
Copy link
Contributor

@etpinard etpinard commented Sep 8, 2016

Setting all scatter traces to visible: false on a given graph at once (e.g. by Plotly.restyle(gd, 'visible', false); is currently broken in master and v1.17.0 ever since commit 57892ed of PR #802. By broken, I meant the visible: false traces remain visible on the graph after the update.

As of 57892ed, scatter traces aren't purged in the cartesian subplot plot step (see here), meaning that the Scatter trace module takes the responsibility of cleaning up visible: false traces - which works fine except when all scatter traces are simultaneously gone. In this case, fullLayout._modules does not contain the Scatter module here and Scatter.plot is not called here.

I'm starting to think that we are not correctly handling visible: false starting from the trace supplyDefaults. Changing that will be a bigger project that I'd call too ambitious for a patch release. So, I propose this fix that uses the plots.cleanPlot step.

@rreusser

- as of PR #802, scatter traces are no longer purged
  in the per-subplot cartesian plot step - to maintain
  consistency between transitions.
- While the scatter plot module can remove individual
  'visible: false' traces, when all scatter traces are restyled
  to 'visible: false' the scatter plot module is currently not called!
@etpinard etpinard added bug something broken status: reviewable labels Sep 8, 2016
}
}
}
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

@mdtusz
Copy link
Contributor

mdtusz commented Sep 9, 2016

Agreed on changing the supplyDefaults step there - we should ensure that each trace has the same "structure" so that will be a good future fix.

💃 from me.

@etpinard etpinard merged commit 923797a into master Sep 9, 2016
@etpinard etpinard deleted the hotfix-visible-false-scatter branch September 9, 2016 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants