-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix: #8659 Number Field Input Validation Update #10362
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
base: main
Are you sure you want to change the base?
Changes from all commits
ba48d58
f524ab6
823b2a2
20bfbed
5dbacba
b4e2a46
2e49430
e1c9ff7
ab91246
d5f14c2
fc83aa9
a031218
198e31d
2fffe4d
cca7bdb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,7 +24,7 @@ import { | |
| } from '@react-types/shared'; | ||
| import {FormValidationState, useFormValidationState} from '../form/useFormValidationState'; | ||
| import {NumberFormatter, NumberParser} from '@internationalized/number'; | ||
| import {useCallback, useMemo, useState} from 'react'; | ||
| import {useCallback, useEffect, useMemo, useRef, useState} from 'react'; | ||
| import {useControlledState} from '../utils/useControlledState'; | ||
|
|
||
| export interface NumberFieldProps | ||
|
|
@@ -185,6 +185,14 @@ export function useNumberFieldState(props: NumberFieldStateOptions): NumberField | |
| value: numberValue | ||
| }); | ||
|
|
||
| let prevControlledValue = useRef(value); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how does this fix work? what is the root cause of the problem? what other approaches were considered? This comment #8659 (comment) made it sound like this was a bigger issue than just NumberField, did you consider the root of all of them for a more holistic approach? |
||
| useEffect(() => { | ||
| if (value !== undefined && !Object.is(value, prevControlledValue.current)) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment (i'm not sure why it's hidden as outdated) made it sound possibly related to focus, but i don't see anything about focus here what did you discover when you looked into this? |
||
| validation.commitValidation(); | ||
| } | ||
| prevControlledValue.current = value; | ||
| }, [value]); | ||
|
|
||
| let clampStep = step !== undefined && !isNaN(step) ? step : 1; | ||
| if (intlOptions.style === 'percent' && (step === undefined || isNaN(step))) { | ||
| clampStep = 0.01; | ||
|
|
||
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.
what's this extra act for? user event uses fireEvent which already wraps everything inside an
actAlso, why are there changes in this file?
usingshould be the correct way to use a spy.If there's something that isn't being awaited, likely it's something leaking from an earlier test into this one.
Whatever it is though, it doesn't give me a lot of confidence in this approach, especially without any explanation.