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

SAA-2291: Introduce convention for strings with number prefix #1108

Merged
merged 6 commits into from
Jan 27, 2025
Merged

Conversation

natclamp-moj
Copy link
Contributor

Identify the bug

#1107

Description of the change

Instead of running all cell values through parseFloat, which converts strings with number prefixes to just the number, we use Number to evaluate the entire value to see whether it's a number or not. If the value is NaN, the function returns the original value (this includes strings with number prefixes) for use in the sort. If the value is a number, we can then safely use parseFloat on the sort value, as we know it won't be removing any non-numerical components of the value.

Alternative designs

Number evaluates the entire value, whereas parseFloat stops when you get to a non-numerical part.
You could use parseFloat and then check the result against the original input value, and use the original value if it is different from the result of the parseFloat, but this is more long-winded and potentially error-prone.

Possible drawbacks

Number converts empty strings and null to 0. I think that this works fine when sorting - if I had a cell which looked empty, I'd rather it was at the bottom of the table rather than at the top - but this depends on your point of view.

Verification process

Verified against existing tests and introduced a new test with a variety of input values.

Release notes

  • Provides a better convention for sorting if strings contain a number prefix

@natclamp-moj natclamp-moj requested a review from a team as a code owner January 22, 2025 13:54
chrispymm
chrispymm previously approved these changes Jan 27, 2025
@chrispymm chrispymm merged commit b424743 into main Jan 27, 2025
22 checks passed
@chrispymm chrispymm deleted the SAA-2291 branch January 27, 2025 14:31
gregtyler pushed a commit that referenced this pull request Feb 24, 2025
# [3.4.0](v3.3.1...v3.4.0) (2025-02-24)

### Bug Fixes

* **sortable table:** improve sorting of strings with a number prefix ([#1108](#1108)) ([b424743](b424743))

### Features

* **alert:** Add alert component, archive banner component ([0f8c6a7](0f8c6a7))
@gregtyler
Copy link
Contributor

🎉 This PR is included in version 3.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

4 participants