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

fix: Sorting issues #1055

Merged
merged 5 commits into from
Jun 6, 2023
Merged

fix: Sorting issues #1055

merged 5 commits into from
Jun 6, 2023

Conversation

bprusinowski
Copy link
Collaborator

@bprusinowski bprusinowski commented Jun 5, 2023

Fixes #1057.
Fixes #1054.
Fixes #1053.

For the line chart legend ordering issues, we were not using getSortingOrders helper function, which lead to orderBy function not using right order for subsequent sorting functions.

For the table sorting problems, we were:

  • expecting that a value will always have either identifier or position (which in not always the case, e.g. for TemporalDimensions) and filtering out such values in makeDimensionValueSorters. This led to broken table sort when sorting e.g. TemporalDimensions. @ptbrowne do you remember why such filtering was introduced in the first place?
  • sorting NumericalMeasures by labels. As we only fetch min and max value for such dimensions, this led to broken sorting logic for every NumericalMeasure (and when the label was there, we were treating it as string, while is should have been treated as number). This PR fixes that issue by returning an identity function (almost, converting string to number) for NumericalMeasures.

How to test

Re-create the charts defined in the issue to see that the sorting works correctly now.

@bprusinowski bprusinowski requested a review from ptbrowne as a code owner June 5, 2023 14:25
@vercel
Copy link

vercel bot commented Jun 5, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
visualization-tool ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 6, 2023 11:44am

Copy link
Collaborator

@ptbrowne ptbrowne left a comment

Choose a reason for hiding this comment

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

Code changes all look OK to me ! Thanks @bprusinowski ! Do you think we could add a test for the nominal values sorting ?

@ptbrowne
Copy link
Collaborator

ptbrowne commented Jun 6, 2023

@ptbrowne do you remember why such filtering was introduced in the first place?

Sadly, I do not remember why there was a sorting on identifier or position in the first place.

@bprusinowski
Copy link
Collaborator Author

@ptbrowne I've added two tests for nominal and measure dimensions 👍

@bprusinowski bprusinowski merged commit 55f8925 into main Jun 6, 2023
@bprusinowski bprusinowski deleted the fix/sorting-issues branch June 6, 2023 11:51
@sosiology
Copy link
Contributor

@ptbrowne do you remember why such filtering was introduced in the first place?

Sadly, I do not remember why there was a sorting on identifier or position in the first place.

Maybe this @bprusinowski #821

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants