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

Add comparator function to ordinal domain sort #152

Merged
merged 7 commits into from
Oct 14, 2023
Merged

Add comparator function to ordinal domain sort #152

merged 7 commits into from
Oct 14, 2023

Conversation

mhkeller
Copy link
Owner

See #151

@mhkeller
Copy link
Owner Author

This also adds a fix where unique dates would not be calculated properly. Now, it compares the dates using .getTime() instead of relying on Set to calculate uniqueness.

@mattlangeman
Copy link

Looks good! I think the two instances of set.add(val) that are outside of the if are not necessary, but otherwise this should fix the odd sort behaviour for dates.

@mhkeller
Copy link
Owner Author

Great thanks for the help in spotting that! I forgot to delete that line from the prior implementation.

Copy link
Contributor

@techniq techniq left a comment

Choose a reason for hiding this comment

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

Thanks @mhkeller so getting to this so quickly. Looks great

src/lib/utils/sorter.js Outdated Show resolved Hide resolved
src/lib/utils/getTime.js Outdated Show resolved Hide resolved
@mattlangeman
Copy link

My other thought would be to tweak the comments to say something like

* Date objects of the same date are not equivalent in a Set
* If val[k] is a date we turn it into a time value with getTime, otherwise getTime returns val[k].
* Push val[k] to results, though, so that we aren't converting the users data.

It took me a bit longer than it should have to understand the if block because I was expecting it to be checking if val[k] was date.

@techniq
Copy link
Contributor

techniq commented Oct 14, 2023

FYI, D3 does something similar with it's InternMap / InternSet (used for group/rollup/etc). Could we use those here as well?

@mhkeller
Copy link
Owner Author

@techniq yea that's also a good idea. Another helpful d3-array function that slipped my mind!

@mhkeller
Copy link
Owner Author

Thanks you both spotting this and your very helpful comments today!

@mhkeller mhkeller merged commit f59eafc into main Oct 14, 2023
5 checks passed
@techniq
Copy link
Contributor

techniq commented Oct 14, 2023

Thanks @mhkeller for the quick fix and awesome library!

@mattlangeman
Copy link

Thanks @mhkeller for the quick fix and thanks @techniq for pointing out the helpful functionality available from d3.

@mhkeller mhkeller deleted the fix-sort branch November 21, 2023 01:26
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.

3 participants