Skip to content

fix for transforms operating on auto-invisible traces #3139

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

Merged
merged 1 commit into from
Oct 23, 2018

Conversation

alexcjohnson
Copy link
Collaborator

Transforms mean supplyDefaults gets run twice. This is mostly fine, but there are some unwanted effects from running it on a trace object that has already had its defaults filled in. The one I'm concerned with here is that if a trace is explicitly visible: false we don't even run the module defaults, but occasionally there are things that automatically trigger visible: false in the module defaults, after we've already set some properties in fullTrace. Because the visible: false only makes sense based on those properties, we still want them to be part of the final trace (for example, to make RCE and toolpanel happy). So in this PR I detect this situation and clear expandedTrace.visible so that the original logic will run again the second time through supplyDefaults.

@etpinard can you think of any unwanted consequences of this? cc @antoinerg @archmoj - one of the darker, dingier corners of plotly.js...

@etpinard
Copy link
Contributor

can you think of any unwanted consequences of this?

Nop, I can't think of any.

Jugging from a quick grep, this splom partial visibility scenario is the only case where we set traceOut.visible to false other than when data arrays are missing or empty.

I'd say 💃

@etpinard
Copy link
Contributor

Referencing #1830, which highlights a similar (very) dark corner of transform-land.

@alexcjohnson alexcjohnson merged commit 117fbaa into master Oct 23, 2018
@alexcjohnson alexcjohnson deleted the transform-visibility branch October 23, 2018 17:14
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