Skip to content

descending shorthand #1591

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

Merged
merged 5 commits into from
May 22, 2023
Merged

descending shorthand #1591

merged 5 commits into from
May 22, 2023

Conversation

mbostock
Copy link
Member

@mbostock mbostock commented May 19, 2023

Fixes #1458. This adds -name shorthand to the mark sort option, applying to both imputed ordinal scale domains and the basic sort transform.

There are still two more places where we could support -name shorthand: the stack transform’s order option, and the bin and group transform’s sort option. I guess I would like to support these, too, but it felt a bit burdensome to make more changes, so I’m hoping this can stand on its own.

@mbostock mbostock requested a review from Fil May 19, 2023 17:39
@Fil
Copy link
Contributor

Fil commented May 21, 2023

It's very nice to use. I hope we can make the extra mile that will make it work everywhere there is sorting/ordering (tentatively #1606).

@mbostock
Copy link
Member Author

This should wait for #1607 (at the very least for better testing, but also because that PR clarifies the desired behavior).

@mbostock mbostock force-pushed the mbostock/descending-shorthand branch from bef0ec9 to 00338c8 Compare May 22, 2023 16:52
@mbostock mbostock requested a review from Fil May 22, 2023 17:29
@mbostock
Copy link
Member Author

Okay, I have merged with #1607, and also made the behavior more conservative: now the -channel shorthand only changes the default order option; it doesn’t attempt to invert the meaning of the given order if one is specified explicitly. I think this is easier to understand and simplifies the implementation slightly since we don’t need the reverseOrder wrapper.

It should be possible to extend this to the stack, group, and bin transforms. For the group and bin transforms, we’ll need some plumbing to change the ascendingDefined here to a dynamic comparator (descendingDefined or something custom):

export function maybeSort(facets, sort, reverse) {
if (sort) {
const S = sort.output.transform();
const compare = (i, j) => ascendingDefined(S[i], S[j]);
facets.forEach((f) => f.sort(compare));
}
if (reverse) {
facets.forEach((f) => f.reverse());
}
}

Similarly for the stack transform we’ll need plumbing to override the ascendingDefined here:

function applyOrder(stacks, O) {
for (const stack of stacks) {
stack.sort((i, j) => ascendingDefined(O[i], O[j]));
}
}

I feel this work is distracting from the release of 0.6.7 and tooltips, so I would ask that we do not work on extending this PR to stack, group, and bin transform today. I would like to get back to writing the documentation for 0.6.7 and determining if there is any additional blocking functionality (such as custom formats for crosshairs #1597).

Copy link
Contributor

@Fil Fil left a comment

Choose a reason for hiding this comment

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

I like how small the code change is. It's mostly docs and examples!

Co-authored-by: Philippe Rivière <[email protected]>
@mbostock mbostock enabled auto-merge (squash) May 22, 2023 19:04
@mbostock mbostock merged commit 3e9497a into main May 22, 2023
@mbostock mbostock deleted the mbostock/descending-shorthand branch May 22, 2023 19:05
chaichontat pushed a commit to chaichontat/plot that referenced this pull request Jan 14, 2024
* descending shorthand

* no double reverse

* minus shorthand only changes default order

* edits

* Update docs/features/scales.md

Co-authored-by: Philippe Rivière <[email protected]>

---------

Co-authored-by: Philippe Rivière <[email protected]>
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.

Shorthand descending sort
2 participants