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/parse number locale #2435

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

vskorohod
Copy link

What I did

  1. Pass the options object to the getParseMode function.
  2. Check the locale option and return 'withLocale' if its not undefined.
  3. Update current unit tests, add new to check different local options.

Copy link

changeset-bot bot commented Jan 3, 2025

🦋 Changeset detected

Latest commit: 10fb755

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@lion/ui Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Contributor

@ryubro ryubro left a comment

Choose a reason for hiding this comment

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

Thank you for the fix!!

I found a few minor issues. Can you have a look?

Also, please add the change log by running npm run changeset.

if (options.locale) {
return 'withLocale';
}
if (options.mode === 'auto' && separators.length === 1) {
const decimalLength = value.split(`${separators}`)[1].length;
if (decimalLength >= 3) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@gerjanvangeest, what was the idea behind this logic? Just wondering, if this logic is still relevant after introducing options.locale.

Copy link
Author

Choose a reason for hiding this comment

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

if you check line 33, with the 55.55 value the decimal length is less than 3 and the function returns ''heuristic", not "withLocale". From my point of view, if we provide the locale, it should always return the 'withLocale' mode.

Copy link
Member

Choose a reason for hiding this comment

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

If you only look at the parser it might make sense to add this attribute. But if i look at the InputAmount, it is a good feature to be a bit more forgiven on the first input.

However when an amount has been formatted once, and then changed the parser could be a bit more strict.

I was thinking about adding another mode version: preformatted.
See the draft pr: #2439

Please let me know what you think about that solution.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @gerjanvangeest

If user changes value from 55.555 to 55.55 with the DE locale, there is a separator.
So these lines will not cover the case, right?

if (!separators || mode === 'preformatted') {
    return 'withLocale';
  }

Copy link
Member

Choose a reason for hiding this comment

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

Its an OR, so that should work as expected.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry. I'm fine with this solution if it helps us to correctly format numbers with the german locale.

@ryubro
Copy link
Contributor

ryubro commented Jan 6, 2025

Also I see some pipeline is failing. Can you have a look, @vskorohod ?

@vskorohod
Copy link
Author

vskorohod commented Jan 7, 2025

Also I see some pipeline is failing. Can you have a look, @vskorohod ?

Hi @ryubro
for lint error:

components/localize/src/number/parseNumber.js:121:48 - error TS2345: Argument of type 'FormatNumberOptions | undefined' is not assignable to parameter of type 'FormatNumberOptions'.
  Type 'undefined' is not assignable to type 'FormatNumberOptions'.

121   const parseMode = getParseMode(cleanedInput, options);

options object should be optional. I made some changes. Could you please take a look?

for browser test error:

 🚧 Browser logs on Firefox:
      Error: could not find controller to hide
        at hide (packages/ui/components/overlays/src/OverlaysManager.js:109:13)
        at hide (packages/ui/components/overlays/src/OverlayController.js:862:20)
        at #outsideEscKeyHandler (packages/ui/components/overlays/src/OverlayController.js:1195:12)
        at mimicEscapePress (packages/ui/components/overlays/test/OverlayController.test.js:34:11)
        at packages/ui/components/overlays/test/OverlayController.test.js:767:25

I have no idea. It doesn't seem to be related to my changes.

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.

4 participants