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

Support z in the difference mark #1906

Merged
merged 9 commits into from
Nov 7, 2023
Merged

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Oct 30, 2023

I'm putting this in a separate PR because it's a bit of code and I'd like a review. (An alternative would be to say we don't support z, but in that case we'd have to be explicitly removing it; the current situation in which we pass z to the area, but merge the clip-paths &c, is halfway and broken.)

In the example chart, we follow three time series (age cohorts) by country, and by sex (the difference makes the chart switch to grey on the only case where young men are less likely than young women of the same cohort to be living with their parents, top-left (Sweden, Y16-19).

For comparison, I'm adding the faceted version (fy: "age").

buggy fixed faceted
young-adults-no-z young-adults young-adults-facet

TODO:

@Fil Fil requested a review from mbostock October 30, 2023 15:56
@Fil Fil changed the title Support z Support z in the difference mark Oct 30, 2023
@mbostock
Copy link
Member

Can we make this look like a more traditional difference chart? I was confused why the line was being drawn in the middle of the filled area, but it’s because the stroke is disabled on the difference mark.

@Fil
Copy link
Contributor Author

Fil commented Oct 31, 2023

good call… it's not even very useful to have the line for T, since it's most probably the midpoint (the numbers of F and M in each cohort being sensibly the same).

Copy link
Member

@mbostock mbostock left a comment

Choose a reason for hiding this comment

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

Rather than repeatedly calling next and grouping by z, I think we just want to call next twice like we do in the parent branch, but collate the clipping paths with the paths. So for example if you have two series, each call to next returns a G element with two path elements; the first path element from the first G becomes the first clipPath element for the first path element in the second G, and so on.

@mbostock mbostock mentioned this pull request Nov 4, 2023
9 tasks
@mbostock mbostock merged commit c7afcb8 into mbostock/difference Nov 7, 2023
@mbostock mbostock deleted the fil/difference branch November 7, 2023 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants