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.

Sign up for GitHub

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

Jump to bottom

Make value and defaultValue valid in number and slider fields #5543

Merged
merged 6 commits into from Dec 14, 2023
Merged

Make value and defaultValue valid in number and slider fields #5543

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1583e45
Make value and defaultValue valid in number and slider fields
snowystinger Dec 7, 2023
98bba1a
Merge branch 'main' into clamp-initial-values-of-number-and-slider
snowystinger Dec 12, 2023
ea83e27
Merge branch 'main' into clamp-initial-values-of-number-and-slider
LFDanLu Dec 14, 2023
9ae7370
Merge branch 'main' into clamp-initial-values-of-number-and-slider
snowystinger Dec 14, 2023
f7d2fcf
Merge branch 'main' into clamp-initial-values-of-number-and-slider
snowystinger Dec 14, 2023
3c7c936
Merge branch 'main' into clamp-initial-values-of-number-and-slider
LFDanLu Dec 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 16 additions & 1 deletion packages/@react-spectrum/numberfield/test/NumberField.test.js
View file
Open in desktop
Expand Up @@ -295,6 +295,21 @@ describe('NumberField', function () {
expect(onChangeSpy.mock.invocationCallOrder[0]).toBeLessThan(onBlurSpy.mock.invocationCallOrder[0]);
});

it.each`
Name | props | expected
${'defaultValue minValue'} | ${{defaultValue: 20, minValue: 50}} | ${'50'}
${'defaultValue maxValue'} | ${{defaultValue: 20, maxValue: 10}} | ${'10'}
${'defaultValue minValue step'} | ${{defaultValue: 20, minValue: 50, step: 3}} | ${'50'}
${'defaultValue maxValue step'} | ${{defaultValue: 20, maxValue: 10, step: 3}} | ${'9'}
${'value minValue'} | ${{value: 20, minValue: 50}} | ${'50'}
${'value maxValue'} | ${{value: 20, maxValue: 10}} | ${'10'}
${'value minValue step'} | ${{value: 20, minValue: 50, step: 3}} | ${'50'}
Copy link
Member

Choose a reason for hiding this comment

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

I've forgotten why this doesn't clamp to 51. Is it that the step interval start at zero or min?

Copy link
Member Author

Choose a reason for hiding this comment

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

steps start at the minValue

${'value maxValue step'} | ${{value: 20, maxValue: 10, step: 3}} | ${'9'}
`('clamps value & defaultValue to the allowed range $Name', function ({props, expected}) {
let {textField} = renderNumberField(props);
expect(textField).toHaveAttribute('value', expected);
});

it.each`
Name
${'NumberField'}
Expand Down Expand Up @@ -1151,7 +1166,7 @@ describe('NumberField', function () {
`('goes to valid step min on `home`', () => {
let {textField} = renderNumberField({onChange: onChangeSpy, defaultValue: 10, step: 10, minValue: -521});

expect(textField).toHaveAttribute('value', '10');
expect(textField).toHaveAttribute('value', '9');
act(() => {textField.focus();});
fireEvent.keyDown(textField, {key: 'Home'});
fireEvent.keyUp(textField, {key: 'Home'});
Expand Down
21 changes: 21 additions & 0 deletions packages/@react-spectrum/slider/test/Slider.test.tsx
View file
Open in desktop
Expand Up @@ -123,6 +123,27 @@ describe('Slider', function () {
expect(output).toHaveTextContent('40');
});

it.each`
Name | props | expected
${'defaultValue minValue'} | ${{defaultValue: 20, minValue: 50}} | ${'50'}
${'defaultValue maxValue'} | ${{defaultValue: 20, maxValue: 10}} | ${'10'}
${'defaultValue minValue step'} | ${{defaultValue: 20, minValue: 50, step: 3}} | ${'50'}
${'defaultValue maxValue step'} | ${{defaultValue: 20, maxValue: 10, step: 3}} | ${'9'}
${'value minValue'} | ${{value: 20, minValue: 50}} | ${'50'}
${'value maxValue'} | ${{value: 20, maxValue: 10}} | ${'10'}
${'value minValue step'} | ${{value: 20, minValue: 50, step: 3}} | ${'50'}
${'value maxValue step'} | ${{value: 20, maxValue: 10, step: 3}} | ${'9'}
`('clamps value & defaultValue to the allowed range $Name', function ({props, expected}) {
let {getByRole} = render(<Slider label="The Label" {...props} />);

let slider = getByRole('slider');
let output = getByRole('status');

expect(slider).toHaveProperty('value', expected);
expect(slider).toHaveAttribute('aria-valuetext', expected);
expect(output).toHaveTextContent(expected);
});

it('can be controlled', function () {
let setValues = [];

Expand Down
20 changes: 20 additions & 0 deletions packages/@react-stately/numberfield/src/useNumberFieldState.ts
View file
Open in desktop
Expand Up @@ -90,6 +90,26 @@ export function useNumberFieldState(
isReadOnly
} = props;

if (value === null) {
value = NaN;
}

if (!isNaN(value)) {
if (!isNaN(step)) {
value = snapValueToStep(value, minValue, maxValue, step);
} else {
value = clamp(value, minValue, maxValue);
}
}
Comment on lines +97 to +103
Copy link
Member

Choose a reason for hiding this comment

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

so this is a departure from native behavior where the initial value provided to the input is respected even if it doesn't match the step but I saw in the spec where it recommends this new behavior so I'm fine with this, feels better anyways


if (!isNaN(defaultValue)) {
if (!isNaN(step)) {
defaultValue = snapValueToStep(defaultValue, minValue, maxValue, step);
} else {
defaultValue = clamp(defaultValue, minValue, maxValue);
}
}

let [numberValue, setNumberValue] = useControlledState<number>(value, isNaN(defaultValue) ? NaN : defaultValue, onChange);
let [inputValue, setInputValue] = useState(() => isNaN(numberValue) ? '' : new NumberFormatter(locale, formatOptions).format(numberValue));

Expand Down
12 changes: 9 additions & 3 deletions packages/@react-stately/slider/src/useSliderState.ts
View file
Open in desktop
Expand Up @@ -13,7 +13,7 @@
import {clamp, snapValueToStep, useControlledState} from '@react-stately/utils';
import {Orientation} from '@react-types/shared';
import {SliderProps} from '@react-types/slider';
import {useMemo, useRef, useState} from 'react';
import {useCallback, useMemo, useRef, useState} from 'react';

export interface SliderState {
/**
Expand Down Expand Up @@ -176,8 +176,14 @@ export function useSliderState<T extends number | number[]>(props: SliderStateOp
return Math.max(calcPageSize, step);
}, [step, maxValue, minValue]);

let value = useMemo(() => convertValue(props.value), [props.value]);
let defaultValue = useMemo(() => convertValue(props.defaultValue) ?? [minValue], [props.defaultValue, minValue]);
let restrictValues = useCallback((values: number[]) => values?.map((val, idx) => {
let min = idx === 0 ? minValue : val[idx - 1];
let max = idx === values.length - 1 ? maxValue : val[idx + 1];
return snapValueToStep(val, min, max, step);
}), [minValue, maxValue, step]);

let value = useMemo(() => restrictValues(convertValue(props.value)), [props.value]);
let defaultValue = useMemo(() => restrictValues(convertValue(props.defaultValue) ?? [minValue]), [props.defaultValue, minValue]);
let onChange = createOnChange(props.value, props.defaultValue, props.onChange);
let onChangeEnd = createOnChange(props.value, props.defaultValue, props.onChangeEnd);

Expand Down