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

Fix parcats + Plotly.react bug #3072

Merged
merged 8 commits into from
Oct 5, 2018
Merged

Fix parcats + Plotly.react bug #3072

merged 8 commits into from
Oct 5, 2018

Conversation

etpinard
Copy link
Contributor

@etpinard etpinard commented Oct 3, 2018

a follow-up on @jonmmease 's #2963, making Plotly.react result in a noop when reacting from the same parcats figure.

I noticed that one of our @noCI was failing, at first I thought that simply appending of test mock list would suffice, but no, we also needed to patch parcats to make that @noCI test pass.

While at it, I also added some tolerance here and there the make the whole suite pass on my setup.

cc @alexcjohnson

... by storing 'calc'ed categoryarray and ticktext as using
    '_', so that Plots.supplyDefaults doesn't pickup spurious changes.
... to make them pass on etpinard's laptop on using Ubuntu 18.04
    and Chrome 69
... to make it pass on etpinard's laptop running Ubuntu 18.04
    and Chrome 69.
@etpinard etpinard added bug something broken type: maintenance labels Oct 3, 2018
@alexcjohnson
Copy link
Collaborator

I also notice we sometimes overwrite displayindex during calc - presumably that can cause problems too?

function validateDimensionDisplayInds(trace) {
var displayInds = trace.dimensions.filter(visible).map(function(dim) {return dim.displayindex;});
if(!isRangePermutation(displayInds)) {
trace.dimensions.filter(visible).forEach(function(dim, i) {
dim.displayindex = i;
});
}
}

@jonmmease
Copy link
Contributor

jonmmease commented Oct 4, 2018 via email

@jonmmease
Copy link
Contributor

Yeah, displayindex is overwritten if the user specified displayindex values are not consistent across dimensions. So perhaps we need to introduce an _displayindex property here as well.

I'm not very familiar with the react pathway yet, but let me know if there's anything here you'd like me to dig into. Thanks @etpinard!

@etpinard
Copy link
Contributor Author

etpinard commented Oct 5, 2018

Yeah, displayindex is overwritten if the user specified displayindex values are not consistent across dimensions. So perhaps we need to introduce an _displayindex property here as well.

I'll take 👀

... so that Plotly.react works on parcats mocks that
    have "bad" displayindex values
@etpinard
Copy link
Contributor Author

etpinard commented Oct 5, 2018

So perhaps we need to introduce an _displayindex property here as well.

Done and 🔒 ed in 157a6e2

@alexcjohnson
Copy link
Collaborator

Thanks for taking this @etpinard - and @jonmmease thanks for chiming in.

Looks great. 💃

@etpinard etpinard merged commit a285aeb into master Oct 5, 2018
@etpinard etpinard deleted the fix-noCI-tests-on-master branch October 5, 2018 18:29
@jonmmease
Copy link
Contributor

Thanks @etpinard!

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.

3 participants