-
Notifications
You must be signed in to change notification settings - Fork 4.6k
ControlWithError: Show validating state when transitioning from error state #71260
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
Changes from 3 commits
dea1939
1046981
ef64fa6
125b759
0adf893
a50671f
ffa9e22
b256311
8f4198b
bc50407
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 |
|---|---|---|
| @@ -0,0 +1,184 @@ | ||
| /** | ||
| * External dependencies | ||
| */ | ||
| import { render, screen, waitFor, act } from '@testing-library/react'; | ||
| import userEvent from '@testing-library/user-event'; | ||
|
|
||
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { useState, useCallback } from '@wordpress/element'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import { ValidatedInputControl } from '../components'; | ||
|
|
||
| describe( 'ControlWithError', () => { | ||
| describe( 'Async Validation', () => { | ||
| beforeEach( () => { | ||
| jest.useFakeTimers(); | ||
| } ); | ||
|
|
||
| afterEach( () => { | ||
| jest.useRealTimers(); | ||
| } ); | ||
|
|
||
| const AsyncValidatedInputControl = ( { | ||
| serverDelayMs, | ||
| }: { | ||
| serverDelayMs: number; | ||
| } ) => { | ||
| const [ text, setText ] = useState( '' ); | ||
| const [ customValidity, setCustomValidity ] = | ||
| useState< | ||
| React.ComponentProps< | ||
| typeof ValidatedInputControl | ||
| >[ 'customValidity' ] | ||
| >( undefined ); | ||
|
|
||
| const onValidate = useCallback( | ||
| ( value?: string ) => { | ||
| setCustomValidity( { | ||
| type: 'validating', | ||
| message: 'Validating...', | ||
| } ); | ||
|
|
||
| // Simulate delayed server response | ||
| setTimeout( () => { | ||
| if ( value?.toLowerCase() === 'error' ) { | ||
| setCustomValidity( { | ||
| type: 'invalid', | ||
| message: 'The word "error" is not allowed.', | ||
| } ); | ||
| } else { | ||
| setCustomValidity( { | ||
| type: 'valid', | ||
| message: 'Validated', | ||
| } ); | ||
| } | ||
| }, serverDelayMs ); | ||
| }, | ||
| [ serverDelayMs ] | ||
| ); | ||
|
|
||
| return ( | ||
| <ValidatedInputControl | ||
| label="Text" | ||
| value={ text } | ||
| onChange={ ( newValue ) => { | ||
| setText( newValue ?? '' ); | ||
| } } | ||
| onValidate={ onValidate } | ||
| customValidity={ customValidity } | ||
| /> | ||
| ); | ||
| }; | ||
|
|
||
| it( 'should not show "validating" state if it takes less than 1000ms', async () => { | ||
| const user = userEvent.setup( { | ||
| advanceTimers: jest.advanceTimersByTime, | ||
| } ); | ||
| render( <AsyncValidatedInputControl serverDelayMs={ 500 } /> ); | ||
|
|
||
| const input = screen.getByRole( 'textbox' ); | ||
|
|
||
| await user.type( input, 'valid text' ); | ||
|
|
||
| // Blur to trigger validation | ||
| await user.tab(); | ||
|
|
||
| // Fast-forward to right before the server response | ||
| act( () => jest.advanceTimersByTime( 499 ) ); | ||
|
|
||
| // The validating state should not be shown | ||
| await waitFor( () => { | ||
| expect( | ||
| screen.queryByText( 'Validating...' ) | ||
| ).not.toBeInTheDocument(); | ||
| } ); | ||
|
|
||
| // Fast-forward past the server delay to show validation result | ||
| act( () => jest.advanceTimersByTime( 1 ) ); | ||
|
|
||
| await waitFor( () => { | ||
| expect( screen.getByText( 'Validated' ) ).toBeInTheDocument(); | ||
|
||
| } ); | ||
| } ); | ||
|
|
||
| it( 'should show "validating" state if it takes more than 1000ms', async () => { | ||
| const user = userEvent.setup( { | ||
| advanceTimers: jest.advanceTimersByTime, | ||
| } ); | ||
| render( <AsyncValidatedInputControl serverDelayMs={ 1200 } /> ); | ||
|
|
||
| const input = screen.getByRole( 'textbox' ); | ||
|
|
||
| await user.type( input, 'valid text' ); | ||
|
|
||
| // Blur to trigger validation | ||
| await user.tab(); | ||
|
|
||
| // Initially, no validating message should be shown (before 1s delay) | ||
| expect( | ||
| screen.queryByText( 'Validating...' ) | ||
| ).not.toBeInTheDocument(); | ||
|
|
||
| // Fast-forward past the 1s delay to show validating state | ||
| act( () => jest.advanceTimersByTime( 1000 ) ); | ||
|
|
||
| await waitFor( () => { | ||
| expect( | ||
| screen.getByText( 'Validating...' ) | ||
| ).toBeInTheDocument(); | ||
| } ); | ||
|
|
||
| // Fast-forward past the server delay to show validation result | ||
| act( () => jest.advanceTimersByTime( 200 ) ); | ||
|
|
||
| await waitFor( () => { | ||
| expect( screen.getByText( 'Validated' ) ).toBeInTheDocument(); | ||
| } ); | ||
|
|
||
| // Test error case | ||
| await user.clear( input ); | ||
| await user.type( input, 'error' ); | ||
|
|
||
| // Blur to trigger validation | ||
| await user.tab(); | ||
|
|
||
| act( () => jest.advanceTimersByTime( 1000 ) ); | ||
|
|
||
| await waitFor( () => { | ||
| expect( | ||
| screen.getByText( 'Validating...' ) | ||
| ).toBeInTheDocument(); | ||
| } ); | ||
|
|
||
| act( () => jest.advanceTimersByTime( 200 ) ); | ||
|
|
||
| await waitFor( () => { | ||
| expect( | ||
| screen.getByText( 'The word "error" is not allowed.' ) | ||
| ).toBeInTheDocument(); | ||
| } ); | ||
|
|
||
| // Test editing after error | ||
| await user.type( input, '{backspace}' ); | ||
|
|
||
| act( () => jest.advanceTimersByTime( 1000 ) ); | ||
|
|
||
| await waitFor( () => { | ||
| expect( | ||
| screen.getByText( 'Validating...' ) | ||
| ).toBeInTheDocument(); | ||
| } ); | ||
|
|
||
| act( () => jest.advanceTimersByTime( 200 ) ); | ||
|
|
||
| await waitFor( () => { | ||
| expect( screen.getByText( 'Validated' ) ).toBeInTheDocument(); | ||
| } ); | ||
| } ); | ||
| } ); | ||
| } ); | ||
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.
Maybe in this case we want to
setErrorMessage( undefined )? There is no error while validation is running.The use case is interaction of the
onValidatecustom (server-side) validation with client-side validation, e.g., when there is something like<input type="email">and the value is not email.In that case, when additional custom validation is in flight, and status is
validating, do we wanterrorMessageto show the "not email" error or not.I'm not sure if we even support this combination of custom and native validation.
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'll be adding tests soon to help document these intended behaviors, but yes, we do support the combination of attribute-based and
customValidity. I tried to match how the native APIs handle it, in that thecustomValiditymessage will be prioritized when it's a non-empty string, but otherwise the rest of the attribute-based validation errors will remain. It also matches the native browser logic of how errors messages are displayed — there can be multiple errors but only one error message is shown at a time.That's a good question. I'm not sure if there's a universally better behavior, but I think in the general case, yes, we do want to show the "not email" error without waiting for the custom validator result.
Let's say there's a 50/50 chance of the custom async validator responding as error, and we wait for the async result before notifying the user about the "not email" error. This means that 50% of the time we made the user wait for no reason, because we already knew about the "not email" error.
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.
Turns out, in practice it did feel better to make the user wait in all cases 😅 Going with
setErrorMessage( undefined )as in your suggestion.