-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
[core] fix(NumericInput): increment/decrement with very small step size #6382
[core] fix(NumericInput): increment/decrement with very small step size #6382
Conversation
@@ -30,7 +30,7 @@ function getDecimalSeparator(locale: string) { | |||
} | |||
|
|||
export function toLocaleString(num: number, locale: string = "en-US") { | |||
return sanitizeNumericInput(num.toLocaleString(locale), locale); | |||
return sanitizeNumericInput(num.toLocaleString(locale, { maximumSignificantDigits: 21 }), locale); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why 21
? according to the NumberFormat docs, this should already be the default. maybe you need to adjust a different option like roundingMode
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was misunderstanding how it works, I opened a task to enhance the docs in this part https://github.com/mdn/mdn/issues/444 😄
|
||
import { NumericInput } from "../../src"; | ||
|
||
describe("<NumericInput>", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are already tests for this component in packages/core/test/controls/numericInputTests.tsx
. can you please move your new tests into that test suite?
@@ -30,7 +30,7 @@ function getDecimalSeparator(locale: string) { | |||
} | |||
|
|||
export function toLocaleString(num: number, locale: string = "en-US") { | |||
return sanitizeNumericInput(num.toLocaleString(locale), locale); | |||
return sanitizeNumericInput(num.toLocaleString(locale, { roundingPriority: "morePrecision" } as any), locale); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Rafael-Martins why do you need to cast as any
? I'd prefer to avoid that cast if we can. If it's absolutely necessary, then it should be accompanied with a // HACKHACK: explanation
comment on the previous line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, sorry, I forgot to send my commentary about it, but the reason is:
I used
as any
here because this property has not included in the type, I was wondering if we have a better way to do it?Reference: microsoft/TypeScript#52072
I will add the HACKHACK or even try a better approach!
@@ -30,7 +30,7 @@ function getDecimalSeparator(locale: string) { | |||
} | |||
|
|||
export function toLocaleString(num: number, locale: string = "en-US") { | |||
return sanitizeNumericInput(num.toLocaleString(locale), locale); | |||
return sanitizeNumericInput(num.toLocaleString(locale, { roundingPriority: "morePrecision" } as any), locale); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used as any
here because this property has not included in the type, I was wondering if we have a better way to do it?
Reference: microsoft/TypeScript#52072
@@ -30,7 +30,7 @@ function getDecimalSeparator(locale: string) { | |||
} | |||
|
|||
export function toLocaleString(num: number, locale: string = "en-US") { | |||
return sanitizeNumericInput(num.toLocaleString(locale), locale); | |||
return sanitizeNumericInput(num.toLocaleString(locale, { maximumSignificantDigits: 21 }), locale); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was misunderstanding how it works, I opened a task to enhance the docs in this part https://github.com/mdn/mdn/issues/444 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @Rafael-Martins!
Fixes #4497
Checklist
Changes proposed in this pull request:
Reviewers should focus on:
Screenshot