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

Axes and grids now support the filter option #1665

Merged
merged 5 commits into from
Jun 28, 2023
Merged

Axes and grids now support the filter option #1665

merged 5 commits into from
Jun 28, 2023

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Jun 1, 2023

The data for the axis and grid marks (the associated scale’s ticks) is computed (in the general case) after the scales have been initialized. As a consequence,

  1. These marks did not support the filter option.
  2. The sort and reverse option crashed as the mark’s data is undefined.

In this PR, by moving the filter logic to their initializer function, we allow the user to filter the axis ticks; for example in usStatePopulationDiverging, to draw ticks on the right only for states with a population decrease, and on the left only for states with a population increase. We also throw a proper error message when the sort and reverse options are specified.

closes #1457
closes #1655

@Fil Fil requested a review from mbostock June 1, 2023 09:56
@Fil
Copy link
Contributor Author

Fil commented Jun 1, 2023

Got side tracked by a typescript issue: I want to remove the first and last ticks of an x axis (as in this D3 horizon chart), and for this I use the third argument ticks that the filter function is getting:

Plot.axisX({filter: (tick, index, ticks) => index > 0 && index < ticks.length - 1})

It works; however it's not documented: the function is typed as ((d: any, i: number) => any) // function of data

EDIT: this secondary remark is now addressed in #1670

@mbostock
Copy link
Member

mbostock commented Jun 1, 2023

@Fil Sounds related to #1640.

Fil added a commit that referenced this pull request Jun 2, 2023
…*, and *data*

previously *data* was present in some cases (when going through the *arraytype*.map code path) but not others (e.g. when doing type conversion)

Addresses this comment in #1665 (comment)
@Fil
Copy link
Contributor Author

Fil commented Jun 2, 2023

the secondary remark is discussed in #1670 and is not blocking for this PR.

Fil added 2 commits June 27, 2023 13:01
Add an explicit error message if using the sort and reverse option

closes #1457
closes #1655
@Fil
Copy link
Contributor Author

Fil commented Jun 27, 2023

rebased

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.

Any context on what the problem was/what you had to change to get it working?

@Fil
Copy link
Contributor Author

Fil commented Jun 27, 2023

The PR’s description is now a bit more explicit.

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.

Thanks for the context. It also helped to hide the whitespace in the diff.

Could we also support sort and reverse by applying them after the axis initializer, like we do for filter? Couldn’t we compose them using the same strategy that the initializer does, just after the axis initializer instead of before? Something like this…

i.initializer = composeInitializer(i.initializer, initializer({filter, sort, reverse}).initializer);

or even shorter

i.initializer = initializer({filter, sort, reverse}, i.initializer).initializer;

src/marks/axis.js Outdated Show resolved Hide resolved
@Fil Fil requested a review from mbostock June 27, 2023 21:37
@Fil Fil enabled auto-merge (squash) June 27, 2023 21:37
test/plots/axis-labels.ts Outdated Show resolved Hide resolved
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.

The reverse option crashes explicit axis The axis mark doesn’t support the filter option
2 participants