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

sankey: compare links in a flow on hover #3730

Merged
merged 5 commits into from Apr 8, 2019

Conversation

Projects
None yet
3 participants
@antoinerg
Copy link
Collaborator

commented Apr 4, 2019

Closes #3322

Screenshot_2019-04-04_17-30-43

  • Add jasmine tests
@antoinerg

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 4, 2019

Right now, only the top link within a flow has its hover label exactly above it. All the ones below will be shifted. Should I change this behavior such that the hovered link always has its hover label above it? I think this would be ideal but I'm not sure how easy/hard it would be to implement. 🤔

// For each related links, create a hoverItem
for(var i = 0; i < d.flow.links.length; i++) {
var link = d.flow.links[i];
if(gd._fullLayout.hovermode === 'closest' && d.link.pointNumber !== link.pointNumber) continue;

This comment has been minimized.

Copy link
@etpinard

etpinard Apr 4, 2019

Member

So, hovermode: 'x' or 'y' enables the "compare" mode? That's a little unfortunate because sankey don't have x nor y axes, but I guess it's not worth adding a hovermode: 'compare' just for sankey.

This comment has been minimized.

Copy link
@etpinard

etpinard Apr 4, 2019

Member

... so I'm ok with this 👌

@etpinard

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

l but I'm not sure how easy/hard it would be to implement

give it a shot! If you think it's the ideal behavior, it's definitely worth spending at least one more day on this feature.

@antoinerg

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 4, 2019

give it a shot! If you think it's the ideal behavior, it's definitely worth spending at least one more day on this feature.

I agree that the current behavior is not good enough. It should be possible to display any hover label via the mouse. For this to work, we need to center it or at least making sure it's not overflowing. Right now it just overflows and thus it becomes invisible:
Screenshot_2019-04-04_18-32-47

@antoinerg

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 5, 2019

With commit f00b542 we can reach all hoverlabels and fix #3730 (comment):
Screenshot_2019-04-05_15-11-28

@etpinard

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

With commit f00b542 we can reach all hoverlabels and fix #3730 (comment):

🎉 🍾 🥇

@etpinard

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

Brilliant work. 💃

@antoinerg antoinerg merged commit 5baa139 into master Apr 8, 2019

10 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: publish Your tests passed on CircleCI!
Details
ci/circleci: test-bundle Your tests passed on CircleCI!
Details
ci/circleci: test-image Your tests passed on CircleCI!
Details
ci/circleci: test-image2 Your tests passed on CircleCI!
Details
ci/circleci: test-jasmine Your tests passed on CircleCI!
Details
ci/circleci: test-jasmine2 Your tests passed on CircleCI!
Details
ci/circleci: test-jasmine3 Your tests passed on CircleCI!
Details
ci/circleci: test-syntax Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details

@antoinerg antoinerg deleted the sankey-multiple-hover branch Apr 8, 2019

@etpinard

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

@antoinerg one sankey @noCI test is failing on master is my machine:

image

Is it passing on your setup?

@archmoj

This comment has been minimized.

Copy link
Collaborator

commented Apr 8, 2019

@etpinard That one also fails on my machine.

@antoinerg

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 8, 2019

It fails sometimes but it does pass more often than not. I guess this one should be a @flaky.

Screenshot_2019-04-08_12-26-23

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.