Skip to content

fix for transforms operating on auto-invisible traces - #3139

Merged
alexcjohnson merged 1 commit into
masterfrom
transform-visibility
Oct 23, 2018
Merged

fix for transforms operating on auto-invisible traces#3139
alexcjohnson merged 1 commit into
masterfrom
transform-visibility

Conversation

@alexcjohnson

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