-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Update NumberInput blur value handling and introduce onStepperBlur #21052
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
b04e0bb
a9f5006
b25430d
c77d7f5
f516ad7
6428220
097230f
7786dc1
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 | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -210,15 +210,19 @@ export interface NumberInputProps | |||||||||||||||||||||
| min?: number; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /** | ||||||||||||||||||||||
| * Provide an optional handler that is called when the input or stepper | ||||||||||||||||||||||
| * buttons are blurred. | ||||||||||||||||||||||
| * Provide an optional handler that is called when the input is blurred. | ||||||||||||||||||||||
|
Contributor
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. could you also update the matching |
||||||||||||||||||||||
| */ | ||||||||||||||||||||||
| onBlur?: ( | ||||||||||||||||||||||
| event: | ||||||||||||||||||||||
| | React.FocusEvent<HTMLInputElement> | ||||||||||||||||||||||
| | React.FocusEvent<HTMLButtonElement> | ||||||||||||||||||||||
| event: React.FocusEvent<HTMLInputElement>, | ||||||||||||||||||||||
| value?: string | number | ||||||||||||||||||||||
| ) => void; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /** | ||||||||||||||||||||||
| * Provide an optional handler that is called when the stepper | ||||||||||||||||||||||
| * buttons are blurred. | ||||||||||||||||||||||
| */ | ||||||||||||||||||||||
| onStepperBlur?: (event: React.FocusEvent<HTMLButtonElement>) => void; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /** | ||||||||||||||||||||||
| * Provide an optional handler that is called when the internal state of | ||||||||||||||||||||||
| * NumberInput changes. This handler is called with event and state info. | ||||||||||||||||||||||
|
|
@@ -394,6 +398,7 @@ const NumberInput = React.forwardRef<HTMLInputElement, NumberInputProps>( | |||||||||||||||||||||
| max, | ||||||||||||||||||||||
| min, | ||||||||||||||||||||||
| onBlur, | ||||||||||||||||||||||
| onStepperBlur, | ||||||||||||||||||||||
| onChange, | ||||||||||||||||||||||
| onClick, | ||||||||||||||||||||||
| onKeyUp, | ||||||||||||||||||||||
|
|
@@ -830,7 +835,15 @@ const NumberInput = React.forwardRef<HTMLInputElement, NumberInputProps>( | |||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| if (onBlur) { | ||||||||||||||||||||||
| onBlur(e); | ||||||||||||||||||||||
| if (type === 'number') { | ||||||||||||||||||||||
| onBlur(e, value); | ||||||||||||||||||||||
| return; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const _numberValue = isControlled | ||||||||||||||||||||||
| ? numberParser.parse(inputValue) | ||||||||||||||||||||||
| : numberValue; | ||||||||||||||||||||||
| onBlur(e, _numberValue); | ||||||||||||||||||||||
|
Comment on lines
+843
to
+846
Contributor
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.
Suggested change
Contributor
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. In the You will need to add:
and add below to |
||||||||||||||||||||||
| } | ||||||||||||||||||||||
| }} | ||||||||||||||||||||||
| pattern={pattern} | ||||||||||||||||||||||
|
|
@@ -858,7 +871,7 @@ const NumberInput = React.forwardRef<HTMLInputElement, NumberInputProps>( | |||||||||||||||||||||
| className={`${prefix}--number__control-btn down-icon`} | ||||||||||||||||||||||
| disabled={disabled || readOnly} | ||||||||||||||||||||||
| onClick={(event) => handleStepperClick(event, 'down')} | ||||||||||||||||||||||
| onBlur={onBlur} | ||||||||||||||||||||||
| onBlur={onStepperBlur} | ||||||||||||||||||||||
| tabIndex={-1} | ||||||||||||||||||||||
| title={decrementNumLabel || iconDescription} | ||||||||||||||||||||||
| type="button"> | ||||||||||||||||||||||
|
|
@@ -870,7 +883,7 @@ const NumberInput = React.forwardRef<HTMLInputElement, NumberInputProps>( | |||||||||||||||||||||
| className={`${prefix}--number__control-btn up-icon`} | ||||||||||||||||||||||
| disabled={disabled || readOnly} | ||||||||||||||||||||||
| onClick={(event) => handleStepperClick(event, 'up')} | ||||||||||||||||||||||
| onBlur={onBlur} | ||||||||||||||||||||||
| onBlur={onStepperBlur} | ||||||||||||||||||||||
| tabIndex={-1} | ||||||||||||||||||||||
| title={incrementNumLabel || iconDescription} | ||||||||||||||||||||||
| type="button"> | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -502,6 +502,66 @@ describe('NumberInput', () => { | |
| expect(input).toHaveValue(15.01); | ||
| }); | ||
|
|
||
| it('should call `onBlur` with value parameter when input is blurred (type="number")', async () => { | ||
|
Contributor
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. Can you add one test that focuses a stepper button and tabs away, asserting Mind running |
||
| const onBlur = jest.fn(); | ||
| render( | ||
| <NumberInput | ||
| label="test-label" | ||
| id="test" | ||
| onBlur={onBlur} | ||
| min={0} | ||
| defaultValue={25} | ||
| max={100} | ||
| translateWithId={translateWithId} | ||
| /> | ||
| ); | ||
|
|
||
| await userEvent.click(screen.getByLabelText('test-label')); | ||
| await userEvent.tab(); | ||
| expect(onBlur).toHaveBeenCalledTimes(1); | ||
| expect(onBlur).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| target: expect.any(Object), | ||
| }), | ||
| 25 | ||
| ); | ||
| }); | ||
|
|
||
| it('should call `onBlur` with parsed value in controlled mode (type="number")', async () => { | ||
| const onBlur = jest.fn(); | ||
| const ControlledNumberInput = () => { | ||
| const [value, setValue] = useState(15); | ||
| return ( | ||
| <NumberInput | ||
| label="test-label" | ||
| id="test" | ||
| onBlur={onBlur} | ||
| value={value} | ||
| onChange={(e, state) => setValue(state.value)} | ||
| min={0} | ||
| max={100} | ||
| translateWithId={translateWithId} | ||
| /> | ||
| ); | ||
| }; | ||
|
|
||
| render(<ControlledNumberInput />); | ||
|
|
||
| const input = screen.getByLabelText('test-label'); | ||
| await userEvent.click(input); | ||
| await userEvent.click(screen.getByLabelText('increment')); | ||
| expect(input).toHaveValue(16); | ||
|
|
||
| await userEvent.click(input); | ||
| await userEvent.tab(); | ||
| expect(onBlur).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| target: expect.any(Object), | ||
| }), | ||
| 16 | ||
| ); | ||
| }); | ||
|
|
||
| describe('with type="text"', () => { | ||
| it('should render an <input> with type="text"', () => { | ||
| render(<NumberInput type="text" label="test-label" id="test" />); | ||
|
|
@@ -743,7 +803,91 @@ describe('NumberInput', () => { | |
| expect(onBlur).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| target: expect.any(Object), | ||
| }) | ||
| }), | ||
| 10 | ||
| ); | ||
| }); | ||
|
|
||
| it('should call `onBlur` with value parameter in uncontrolled mode', async () => { | ||
| const onBlur = jest.fn(); | ||
| render( | ||
| <NumberInput | ||
| type="text" | ||
| label="test-label" | ||
| id="test" | ||
| onBlur={onBlur} | ||
| min={0} | ||
| defaultValue={42} | ||
| max={100} | ||
| translateWithId={translateWithId} | ||
| /> | ||
| ); | ||
|
|
||
| await userEvent.click(screen.getByLabelText('test-label')); | ||
| await userEvent.tab(); | ||
| expect(onBlur).toHaveBeenCalledTimes(1); | ||
| expect(onBlur).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| target: expect.any(Object), | ||
| }), | ||
| 42 | ||
| ); | ||
| }); | ||
|
|
||
| it('should call `onBlur` with updated value after user types', async () => { | ||
| const onBlur = jest.fn(); | ||
| render( | ||
| <NumberInput | ||
| type="text" | ||
| label="test-label" | ||
| id="test" | ||
| onBlur={onBlur} | ||
| min={0} | ||
| defaultValue={10} | ||
| max={100} | ||
| translateWithId={translateWithId} | ||
| /> | ||
| ); | ||
|
|
||
| const input = screen.getByLabelText('test-label'); | ||
| await userEvent.clear(input); | ||
| await userEvent.type(input, '75'); | ||
| await userEvent.tab(); | ||
|
|
||
| expect(onBlur).toHaveBeenCalledTimes(1); | ||
| expect(onBlur).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| target: expect.any(Object), | ||
| }), | ||
| 75 | ||
| ); | ||
| }); | ||
|
|
||
| it('should call `onBlur` with NaN when value is empty and allowEmpty is true', async () => { | ||
| const onBlur = jest.fn(); | ||
| render( | ||
| <NumberInput | ||
| type="text" | ||
| label="test-label" | ||
| id="test" | ||
| onBlur={onBlur} | ||
| allowEmpty | ||
| min={0} | ||
| max={100} | ||
| translateWithId={translateWithId} | ||
| /> | ||
| ); | ||
|
|
||
| const input = screen.getByLabelText('test-label'); | ||
| await userEvent.click(input); | ||
| await userEvent.tab(); | ||
|
|
||
| expect(onBlur).toHaveBeenCalledTimes(1); | ||
| expect(onBlur).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| target: expect.any(Object), | ||
| }), | ||
| NaN | ||
| ); | ||
| }); | ||
|
|
||
|
|
@@ -773,7 +917,7 @@ describe('NumberInput', () => { | |
| expect(screen.getByLabelText('test-label')).toHaveValue('10'); | ||
| }); | ||
|
|
||
| it('should call `onBlur` when focus leaves the input, decrement button, or increment button', async () => { | ||
| it('should call `onBlur` when focus leaves the input with the values set by decrement button, or increment button', async () => { | ||
| const onBlur = jest.fn(); | ||
|
|
||
| render( | ||
|
|
@@ -789,35 +933,35 @@ describe('NumberInput', () => { | |
| /> | ||
| ); | ||
|
|
||
| await userEvent.click(screen.getByLabelText('test-label')); | ||
| expect(screen.getByLabelText('test-label')).toHaveFocus(); | ||
| const input = screen.getByLabelText('test-label'); | ||
| await userEvent.click(input); | ||
| expect(input).toHaveFocus(); | ||
|
|
||
| await userEvent.click(screen.getByLabelText('decrement')); | ||
| expect(onBlur).toHaveBeenCalledTimes(1); | ||
| expect(onBlur).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| target: expect.any(Object), | ||
| }) | ||
| ); | ||
| expect(screen.getByLabelText('decrement')).toHaveFocus(); | ||
|
|
||
| await userEvent.click(screen.getByLabelText('increment')); | ||
| expect(onBlur).toHaveBeenCalledTimes(2); | ||
| await userEvent.click(input); | ||
| await userEvent.tab(); | ||
|
|
||
| expect(onBlur).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| target: expect.any(Object), | ||
| }) | ||
| }), | ||
| 9 | ||
| ); | ||
|
|
||
| await userEvent.click(screen.getByLabelText('increment')); | ||
| expect(screen.getByLabelText('increment')).toHaveFocus(); | ||
|
|
||
| await userEvent.click(screen.getByLabelText('test-label')); | ||
| expect(onBlur).toHaveBeenCalledTimes(3); | ||
| await userEvent.click(input); | ||
| await userEvent.tab(); | ||
|
|
||
| expect(onBlur).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| target: expect.any(Object), | ||
| }) | ||
| }), | ||
| 10 | ||
| ); | ||
| expect(screen.getByLabelText('test-label')).toHaveFocus(); | ||
| }); | ||
|
|
||
| it('should set up and down arrows as disabled if `disabled` is true', () => { | ||
|
|
@@ -1511,6 +1655,42 @@ describe('NumberInput', () => { | |
| expect(screen.getByText('test-invalid-text')).toBeInTheDocument(); | ||
| expect(screen.getByRole('textbox')).toHaveAttribute('data-invalid'); | ||
| }); | ||
|
|
||
| it('should call `onBlur` with parsed value in controlled mode', async () => { | ||
| const onBlur = jest.fn(); | ||
| const ControlledNumberInput = () => { | ||
| const [value, setValue] = useState(20.4); | ||
| return ( | ||
| <NumberInput | ||
| type="text" | ||
| label="test-label" | ||
| id="test" | ||
| onBlur={onBlur} | ||
| value={value} | ||
| onChange={(e, state) => setValue(state.value)} | ||
| min={0} | ||
| max={100} | ||
| step={1} | ||
| translateWithId={translateWithId} | ||
| /> | ||
| ); | ||
| }; | ||
|
|
||
| render(<ControlledNumberInput />); | ||
|
|
||
| const input = screen.getByLabelText('test-label'); | ||
| await userEvent.click(input); | ||
| expect(input).toHaveValue('20.4'); | ||
|
|
||
| await userEvent.tab(); | ||
| expect(onBlur).toHaveBeenCalledTimes(1); | ||
| expect(onBlur).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| target: expect.any(Object), | ||
| }), | ||
| 20.4 | ||
| ); | ||
| }); | ||
| }); | ||
| }); | ||
| }); | ||
Uh oh!
There was an error while loading. Please reload this page.
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'm not sure about the
console log, because it nudges consumers toward devtools. Could we swap it for a Storybook action instead?storybook action arg
import { action } from '@storybook/addon-actions';and thenonBlur={action('onBlur')}.That way the blur value shows up in the Actions panel without changing the UI.