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 box & violin inner parts removal #2785

Merged
merged 5 commits into from
Jul 6, 2018
Merged

Conversation

etpinard
Copy link
Contributor

@etpinard etpinard commented Jul 5, 2018

fixes #2777 and a bunch of other restyle/react bugs introduced in #2579, where the box and violin meanlines and points d3-data-binds assume a clean parent <g> and didn't care about clearing parts that should no longer be visible.

cc @alexcjohnson @dmt0

@etpinard etpinard added bug something broken status: reviewable labels Jul 5, 2018
fn = Lib.identity;
}

var paths = sel.selectAll('path.box').data(fn || []);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like this pattern! But two comments about it:

  • trace.box and trace.meanline are deleted rather than left with visible: false - so can't we just say (trace.type === 'violin' && trace.box) etc?
  • I'm a bit uneasy relying on uninitialized vars being undefined - seems like it makes things more fragile, for example if we use fn again, or wrap this block in a loop or something... I'd rather a pattern like
var fn;
if(...) fn = Lib.identity;
else fn = []; // or just var fn = []; and drop the else, dunno which is better

var paths = sel.selectAll('path.box').data(fn);

@@ -102,6 +102,7 @@ function plotBoxAndWhiskers(sel, axes, trace, t) {

var fn;
if(trace.type === 'box' ||
trace.type === 'candlestick' ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps trace.type !== 'violin' || trace.box? NBD either way, we'd of course figure it out pretty quick if we add any other trace type that reuses box drawing.

@alexcjohnson
Copy link
Collaborator

Very nicely done - getting us that much closer to regular d3 idioms - and great tests! Both of my comments are nonblocking - 💃

@etpinard etpinard merged commit 7e1c98c into master Jul 6, 2018
@etpinard etpinard deleted the box-violin-innerpart-removal branch July 6, 2018 22:50
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.

violin: hiding a box within violin produces a black box
2 participants