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

Add polar.hole #2977

Merged
merged 13 commits into from
Sep 10, 2018
Merged

Add polar.hole #2977

merged 13 commits into from
Sep 10, 2018

Conversation

etpinard
Copy link
Contributor

@etpinard etpinard commented Sep 6, 2018

checking off the third item on the #2255 open-item list

peek 2018-09-06 17-51

Commit-by-commit:

  • 39b2ee3 fixes barpolar + scatterpolar ordering (I'm 😄 to have caught that one before v1.41.0)
  • ea7fd2c adds polar.hole along with a test mock
  • f547692 adds a few TODO comments that could be taken care in this PR if they get enough 👍 s from the reviewers

I'll add a few hover tests on subplots with holes and maybe a zoombox test tomorrow morning.

cc @alexcjohnson @antoinerg

- draw subplot with pathAnnulus when polar.hole > 0
- update r range -> x/y range scales
- draw angular grid line from innerRadius w/o translating them
- constrain handles between innerRadius & radius on main drag
- add mock
@etpinard etpinard added bug something broken feature something new status: in progress labels Sep 6, 2018
@etpinard etpinard added this to the v1.41.0 milestone Sep 6, 2018
@@ -937,6 +940,8 @@ proto.updateRadialDrag = function(fullLayout) {
var tx = cx + (radius + bl2) * Math.cos(angle0);
var ty = cy - (radius + bl2) * Math.sin(angle0);

// TODO add 'inner' drag box when innerRadius > 0 !!
Copy link
Collaborator

Choose a reason for hiding this comment

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

not quite sure what this means, but if it's something like the clamped inner edge still jumps to be a circle at the center then yeah, we'd better avoid that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This TODO refers to the radial drag box. Currently we have 1 radial drag box per polar subplot out beyond the subplot radius. We don't drag one at the other end of the radial axis as it would conflict with the "main" drag over the subplot.

peek 2018-09-07 12-22

But, when polar.hole > 0 we could add an inner radial drag box that doesn't conflict with the main drag. This inner radial drag box would update radialaxis.range[0].

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh right! Yes, we should add that. Would be confusing if there's room for one but it's not there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

peek 2018-09-07 16-41

I'm really liking this effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in d466146

@alexcjohnson
Copy link
Collaborator

@etpinard beautifully done. Nice catch with the bar/scatter ordering, looks like we missed that before because the traces in the related mock are semi-transparent...

I like the way you collected TODO items in a commit at the end. Not sure if I'm organized enough to do that myself but I can try 😅

@etpinard etpinard mentioned this pull request Sep 7, 2018
12 tasks
@etpinard
Copy link
Contributor Author

etpinard commented Sep 7, 2018

Tagging as reviewable.

so that:
- we don't have to worry about ordering on relayouts
- subplots on non-full-circle sectors can have functioning
  inner radial dragboxes.
@alexcjohnson
Copy link
Collaborator

💃

@etpinard etpinard merged commit 2a667d0 into master Sep 10, 2018
@etpinard etpinard deleted the polar-hole branch September 10, 2018 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants