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: render a thin but visible node even if value is very low #2017

Merged
merged 1 commit into from
Sep 20, 2017

Conversation

monfera
Copy link
Contributor

@monfera monfera commented Sep 19, 2017

Fixes #2015 (see before/after example there)
Issue found when looking at #2014

Jasmine test produces this DOM tree:

image

@monfera monfera added status: reviewable bug something broken labels Sep 19, 2017
@monfera monfera self-assigned this Sep 19, 2017
@alexcjohnson
Copy link
Collaborator

Good catch, and really nice test!
💃

@alexcjohnson
Copy link
Collaborator

though - just a nitpick, but it doesn't look like the node is aligned quite properly with the link in this case?

@monfera
Copy link
Contributor Author

monfera commented Sep 19, 2017

I can put a bit more into alignment but maybe it could be an independent ticket:

The alignments, in general, aren't perfect. When developing the sankey, there were tradeoffs due to these factors:

  • crisp, uniform thickness node borders requires that SVG shape-rendering is set to crispEdges - compare Mike's original with our take which gives more consistent apparent box borders
  • even if we have this, constant node border (stroke) thickness and constant color requires that the rect borders are snapped - for example, moving a line half a pixel can result in a less contrasty, blurred line (it's done with things like adding 0.5px and ceil/truncate)
  • there were some cross-browser inconsistencies (can't remember)
  • d3-sankey is a black box and returns node geometry and link geometry separately (also, it has no awareness of pixels or snapping)
  • unlike d3-sankey, we more clearly support multiedges - for this reason, we fill shade rather than stroke shade the links - and various options lead to various overlaps between adjacent multiedges, so if we further tweak alignment, we also need to ensure there's no negative banding artifact with multiedges

@alexcjohnson
Copy link
Collaborator

@monfera I see what you're talking about with regular (non-tiny) nodes, and I can see why you did it the way you did. I think the normal case is fine, strikes a good balance between the competing visual requirements. But the new tiny one really seems to be hanging off the bottom of the link. I think it's worth spending a few minutes fixing it just in this special case.

@monfera
Copy link
Contributor Author

monfera commented Sep 19, 2017

@alexcjohnson ah got it, will do!

@alexcjohnson
Copy link
Collaborator

#2021 addresses my comment, so this should be good to go! 💃

@monfera monfera merged commit a307331 into master Sep 20, 2017
@monfera monfera deleted the sankey-minimum-node-size branch September 20, 2017 20:34
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