Skip to content

Fix dendrogram #1063

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 3 commits into from
Oct 24, 2016
Merged

Fix dendrogram #1063

merged 3 commits into from
Oct 24, 2016

Conversation

etpinard
Copy link
Contributor

@etpinard etpinard commented Oct 20, 2016

PR #946 broke a few funky layout configurations.

Take for example this dendrogram: https://plot.ly/~talgalili/23

This fix is simple. Make sure that all Axes.gertSuplots, including the (new) linkSubplots, use fullData if present on-par with what the subplot-layer-creation routine expects.

- no need to call getSubplots twice, grab subplot ids
  from _plots hash for speed.
- which is a more precise description than data,
  after than defaults step.
@etpinard etpinard added bug something broken status: reviewable labels Oct 20, 2016
@etpinard etpinard added this to the v1.19.0 milestone Oct 20, 2016
@@ -1207,7 +1207,7 @@ axes.getSubplots = function(gd, ax) {
var i, j, sp;

// look for subplots in the data
var data = gd.data || [];
var data = gd._fullData || gd.data || [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

nonblocking, but do we even need gd.data here then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't want to break

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah good call, forgot streambed might still be using this 👍

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

Nice and easy 💃 !
Thanks for including the dendrogram test, definitely something we should have,

@etpinard etpinard merged commit ee989d9 into master Oct 24, 2016
@etpinard etpinard deleted the fix-cartesian-funky-layout branch October 24, 2016 15: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