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

Ternary axis layer #1952

Merged
merged 3 commits into from
Aug 16, 2017
Merged

Ternary axis layer #1952

merged 3 commits into from
Aug 16, 2017

Conversation

etpinard
Copy link
Contributor

Implements #1871 for ternary subplots

- to do so, rm <g axlines> and construct plot-layer data
  based on ax.layer
- make sure layer order is updated on ternary.plot using
  selection.order()
- grab layer ref from `this.layers` when possible, to
  undo useless querySelector calls.
@etpinard etpinard added this to the v1.30.0 milestone Aug 15, 2017
@etpinard
Copy link
Contributor Author

I still have to add a few jasmine tests. I'm opening this PR early to tag it under the v1.30.0 milestone.


_this.plotContainer.selectAll('.backplot,.grids')
.call(Drawing.setClipUrl, clipId);
toplevel.order();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

dunno if selection.order wasn't in d3 back when I started using it or I just couldn't find it... but we need to use this maaaany more places in our code!

'aaxis', 'baxis', 'caxis', 'axlines'
];

var plotLayers = ['draglayer', 'plotbg', 'backplot', 'grids'];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The handful of lines below are a little bit verbose. If anyone knows a cleaner / better way to do this, let me know.

Copy link
Collaborator

Choose a reason for hiding this comment

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

dunno if it's worthwhile - shorter but not necessarily cleaner - but you could do something like

var plotOverLayers = ['frontplot'];
['a','b','c'].forEach(function(axLetter) {
  var axName = axLetter + 'axis';
  var isBelow = ternaryLayout[axName].layer === 'below';
  (isBelow ? plotLayers : plotOverLayers).push(axName, axLetter + 'line');
});
[].push.apply(plotLayers, plotOverLayers);

@etpinard
Copy link
Contributor Author

Looks like CircleCI is down for the moment. Nonetheless, type: reviewable.

@etpinard
Copy link
Contributor Author

cc @geocosmite

@alexcjohnson
Copy link
Collaborator

The cliponaxis: true trace with axis.layer: 'below traces' in the new test image looks funny, how the markers cover up half the axis line. But I think trying to solve that - moving the edge of the clip path back to the edge of the axis line, for example - would cause more problems than it solves. This combination doesn't have a use case that's obvious to me, but if you moved the clip path back it would make a subpixel gap so the more common use case cliponaxis: true and axis.layer: 'above traces' would look bad.

So lets keep it as is. No more comments, the one above is arguably not even an improvement 🙈
💃

@etpinard etpinard merged commit 885c545 into master Aug 16, 2017
@etpinard etpinard deleted the ternary-axis-layer branch August 16, 2017 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants