Skip to content

Commit 85bbd1d

Browse files
rebeccaalpertrebeccaalpert
committed
Address feedback
1 parent 24b0a32 commit 85bbd1d

8 files changed

Lines changed: 281 additions & 93 deletions

File tree

‎packages/react-core/src/components/Slider/Slider.tsx‎

Lines changed: 38 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,12 @@ import { TextInput } from '../TextInput';
77
import { Tooltip, TooltipProps } from '../Tooltip';
88
import cssSliderValue from '@patternfly/react-tokens/dist/esm/c_slider_value';
99
import cssFormControlWidthChars from '@patternfly/react-tokens/dist/esm/c_slider__value_c_form_control_width_chars';
10-
import { formatLocalizedDecimal, getLanguageDirection, parseLocalizedDecimal } from '../../helpers/util';
10+
import {
11+
formatLocalizedDecimal,
12+
getLanguageDirection,
13+
getLocalizedInputWidthChars,
14+
parseLocalizedDecimal
15+
} from '../../helpers/util';
1116

1217
/** Properties for creating custom steps in a slider. These properties should be passed in as
1318
* an object within an array to the slider component's customSteps property.
@@ -134,12 +139,17 @@ export const Slider: React.FunctionComponent<SliderProps> = ({
134139
const sliderRailRef = useRef<HTMLDivElement>(undefined);
135140
const thumbRef = useRef<HTMLDivElement>(undefined);
136141
const isInputFocusedRef = useRef(false);
137-
let formatter: Intl.NumberFormat;
138-
try {
139-
formatter = new Intl.NumberFormat(locale);
140-
} catch {
141-
formatter = new Intl.NumberFormat(); // grabs default locale
142-
}
142+
const formatter = useMemo(() => {
143+
try {
144+
return new Intl.NumberFormat(locale, { useGrouping: false });
145+
} catch (error) {
146+
if (error instanceof RangeError) {
147+
// this is what happens if a bad locale is passed in
148+
return new Intl.NumberFormat(undefined, { useGrouping: false });
149+
}
150+
throw error;
151+
}
152+
}, [locale]);
143153

144154
const [localValue, setValue] = useState(value);
145155
const [localInputValue, setLocalInputValue] = useState(inputValue);
@@ -155,9 +165,12 @@ export const Slider: React.FunctionComponent<SliderProps> = ({
155165
setValue(value);
156166
}, [value]);
157167

158-
const updateInputDisplay = useCallback((numericValue: number) => {
159-
setInputDisplayValue(formatLocalizedDecimal(numericValue, formatter));
160-
}, []);
168+
const updateInputDisplay = useCallback(
169+
(numericValue: number) => {
170+
setInputDisplayValue(formatLocalizedDecimal(numericValue, formatter));
171+
},
172+
[formatter]
173+
);
161174

162175
const updateInputFromSliderValue = useCallback(
163176
(numericValue: number) => {
@@ -201,8 +214,15 @@ export const Slider: React.FunctionComponent<SliderProps> = ({
201214
// calculate style value percentage
202215
const stylePercent = ((localValue - min) * 100) / (max - min);
203216
const style = { [cssSliderValue.name]: `${stylePercent}%` } as React.CSSProperties;
204-
const widthChars = useMemo(() => inputDisplayValue.length || 1, [inputDisplayValue]);
205-
const inputStyle = { [cssFormControlWidthChars.name]: widthChars } as React.CSSProperties;
217+
const inputStyle = useMemo(() => {
218+
if (!isInputVisible) {
219+
return undefined;
220+
}
221+
222+
const widthChars = getLocalizedInputWidthChars(formatter, min, max, inputDisplayValue);
223+
224+
return { [cssFormControlWidthChars.name]: widthChars } as React.CSSProperties;
225+
}, [isInputVisible, formatter, min, max, inputDisplayValue]);
206226

207227
const onChangeHandler = (event: React.FormEvent<HTMLInputElement>, value: string) => {
208228
setInputDisplayValue(value);
@@ -312,9 +332,10 @@ export const Slider: React.FunctionComponent<SliderProps> = ({
312332
if (snapValue && !areCustomStepsContinuous) {
313333
thumbRef.current.style.setProperty(cssSliderValue.name, `${snapValue}%`);
314334
setValue(snapValue);
315-
updateInputFromSliderValue(snapValue);
316335
if (onChange) {
317336
onChange(e, snapValue, undefined, setLocalInputValueWithDisplay);
337+
} else {
338+
updateInputFromSliderValue(snapValue);
318339
}
319340
}
320341
};
@@ -382,10 +403,11 @@ export const Slider: React.FunctionComponent<SliderProps> = ({
382403

383404
// Call onchange callback
384405
const resolvedValue = snapValue !== undefined ? snapValue : newValue;
385-
updateInputFromSliderValue(resolvedValue);
386406

387407
if (onChange) {
388408
onChange(e, resolvedValue, undefined, setLocalInputValueWithDisplay);
409+
} else {
410+
updateInputFromSliderValue(resolvedValue);
389411
}
390412
};
391413

@@ -452,9 +474,10 @@ export const Slider: React.FunctionComponent<SliderProps> = ({
452474
if (newValue !== localValue) {
453475
thumbRef.current.style.setProperty(cssSliderValue.name, `${newValue}%`);
454476
setValue(newValue);
455-
updateInputFromSliderValue(newValue);
456477
if (onChange) {
457478
onChange(e, newValue, undefined, setLocalInputValueWithDisplay);
479+
} else {
480+
updateInputFromSliderValue(newValue);
458481
}
459482
}
460483
};

‎packages/react-core/src/components/Slider/__tests__/Slider.test.tsx‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -145,3 +145,9 @@ test('accepts comma decimal input and normalizes on blur', async () => {
145145
expect(input).toHaveValue('62,5');
146146
expect(onChange).toHaveBeenLastCalledWith(expect.any(Object), 50, 62.5, expect.any(Function));
147147
});
148+
149+
test('renders with invalid locale using default number formatting', () => {
150+
render(<Slider value={50.2} isInputVisible inputValue={50.2} locale="invalid-locale-tag" />);
151+
152+
expect(screen.getByRole('textbox', { name: 'Slider value input' })).toHaveValue('50.2');
153+
});

‎packages/react-core/src/components/Slider/__tests__/__snapshots__/Slider.test.tsx.snap‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ exports[`slider renders continuous slider 1`] = `
44
<DocumentFragment>
55
<div
66
class="pf-v6-c-slider"
7-
style="--pf-v6-c-slider--value: 50%; --pf-v6-c-slider__value--c-form-control--width-chars: 2;"
7+
style="--pf-v6-c-slider--value: 50%; --pf-v6-c-slider__value--c-form-control--width-chars: 3;"
88
>
99
<div
1010
class="pf-v6-c-slider__main"
@@ -79,7 +79,7 @@ exports[`slider renders continuous slider with custom steps 1`] = `
7979
<DocumentFragment>
8080
<div
8181
class="pf-v6-c-slider"
82-
style="--pf-v6-c-slider--value: 50%; --pf-v6-c-slider__value--c-form-control--width-chars: 1;"
82+
style="--pf-v6-c-slider--value: 50%;"
8383
>
8484
<div
8585
class="pf-v6-c-slider__main"
@@ -142,7 +142,7 @@ exports[`slider renders disabled slider 1`] = `
142142
<DocumentFragment>
143143
<div
144144
class="pf-v6-c-slider pf-m-disabled"
145-
style="--pf-v6-c-slider--value: 50%; --pf-v6-c-slider__value--c-form-control--width-chars: 1;"
145+
style="--pf-v6-c-slider--value: 50%;"
146146
>
147147
<div
148148
class="pf-v6-c-slider__main"
@@ -199,7 +199,7 @@ exports[`slider renders discrete slider 1`] = `
199199
<DocumentFragment>
200200
<div
201201
class="pf-v6-c-slider"
202-
style="--pf-v6-c-slider--value: 40%; --pf-v6-c-slider__value--c-form-control--width-chars: 2;"
202+
style="--pf-v6-c-slider--value: 40%; --pf-v6-c-slider__value--c-form-control--width-chars: 3;"
203203
>
204204
<div
205205
class="pf-v6-c-slider__main"
@@ -274,7 +274,7 @@ exports[`slider renders discrete slider with custom steps 1`] = `
274274
<DocumentFragment>
275275
<div
276276
class="pf-v6-c-slider"
277-
style="--pf-v6-c-slider--value: 50%; --pf-v6-c-slider__value--c-form-control--width-chars: 1;"
277+
style="--pf-v6-c-slider--value: 50%;"
278278
>
279279
<div
280280
class="pf-v6-c-slider__main"
@@ -366,7 +366,7 @@ exports[`slider renders slider with input 1`] = `
366366
<DocumentFragment>
367367
<div
368368
class="pf-v6-c-slider"
369-
style="--pf-v6-c-slider--value: 50%; --pf-v6-c-slider__value--c-form-control--width-chars: 2;"
369+
style="--pf-v6-c-slider--value: 50%; --pf-v6-c-slider__value--c-form-control--width-chars: 3;"
370370
>
371371
<div
372372
class="pf-v6-c-slider__main"
@@ -458,7 +458,7 @@ exports[`slider renders slider with input above thumb 1`] = `
458458
<DocumentFragment>
459459
<div
460460
class="pf-v6-c-slider"
461-
style="--pf-v6-c-slider--value: 50%; --pf-v6-c-slider__value--c-form-control--width-chars: 2;"
461+
style="--pf-v6-c-slider--value: 50%; --pf-v6-c-slider__value--c-form-control--width-chars: 3;"
462462
>
463463
<div
464464
class="pf-v6-c-slider__main"
@@ -550,7 +550,7 @@ exports[`slider renders slider with input actions 1`] = `
550550
<DocumentFragment>
551551
<div
552552
class="pf-v6-c-slider"
553-
style="--pf-v6-c-slider--value: 50%; --pf-v6-c-slider__value--c-form-control--width-chars: 1;"
553+
style="--pf-v6-c-slider--value: 50%;"
554554
>
555555
<div
556556
class="pf-v6-c-slider__actions"
@@ -631,7 +631,7 @@ exports[`slider renders slider with tooltip on thumb 1`] = `
631631
<DocumentFragment>
632632
<div
633633
class="pf-v6-c-slider"
634-
style="--pf-v6-c-slider--value: 50%; --pf-v6-c-slider__value--c-form-control--width-chars: 1;"
634+
style="--pf-v6-c-slider--value: 50%;"
635635
>
636636
<div
637637
class="pf-v6-c-slider__main"

‎packages/react-core/src/components/Slider/examples/Slider.md‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,14 @@ import RhUiUnlockFillIcon from '@patternfly/react-icons/dist/esm/icons/rh-ui-unl
3232

3333
```
3434

35+
### With localized input
36+
37+
To format and parse decimal values using locale-specific separators, use the `locale` prop with a BCP 47 language tag. For example, German (`de-DE`) and Spanish (`es-ES`) use a comma, while English (`en-US`) uses a period.
38+
39+
```ts file="./SliderValueLocalizedInput.tsx"
40+
41+
```
42+
3543
### Thumb value input
3644

3745
```ts file="./SliderThumbValueInput.tsx"

‎packages/react-core/src/components/Slider/examples/SliderValueLocalizedInput.tsx‎

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
import { useState } from 'react';
2+
import { Content, Slider, SliderOnChangeEvent } from '@patternfly/react-core';
3+
4+
const boundarySteps = [
5+
{ value: 0, label: '0' },
6+
{ value: 1, label: '1' }
7+
];
8+
9+
export const SliderValueLocalizedInput: React.FunctionComponent = () => {
10+
const [valueDe, setValueDe] = useState(0.6);
11+
const [inputValueDe, setInputValueDe] = useState(0.6);
12+
const [valueEn, setValueEn] = useState(0.5);
13+
const [inputValueEn, setInputValueEn] = useState(0.5);
14+
15+
const onChangeDe = (
16+
_event: SliderOnChangeEvent,
17+
value: number,
18+
inputValue: number,
19+
setLocalInputValue: React.Dispatch<React.SetStateAction<number>>
20+
) => {
21+
if (inputValue === undefined) {
22+
setValueDe(value);
23+
setInputValueDe(value);
24+
} else {
25+
const newValue = Math.min(1, Math.max(0, inputValue));
26+
if (newValue !== inputValue) {
27+
setLocalInputValue(newValue);
28+
}
29+
setValueDe(newValue);
30+
setInputValueDe(newValue);
31+
}
32+
};
33+
34+
const onChangeEn = (
35+
_event: SliderOnChangeEvent,
36+
value: number,
37+
inputValue: number,
38+
setLocalInputValue: React.Dispatch<React.SetStateAction<number>>
39+
) => {
40+
if (inputValue === undefined) {
41+
setValueEn(value);
42+
setInputValueEn(value);
43+
} else {
44+
const newValue = Math.min(1, Math.max(0, inputValue));
45+
if (newValue !== inputValue) {
46+
setLocalInputValue(newValue);
47+
}
48+
setValueEn(newValue);
49+
setInputValueEn(newValue);
50+
}
51+
};
52+
53+
return (
54+
<>
55+
<Content component="small">German locale (de-DE) - uses a comma as the decimal separator</Content>
56+
<Slider
57+
value={valueDe}
58+
min={0}
59+
max={1}
60+
step={0.1}
61+
isInputVisible
62+
inputValue={inputValueDe}
63+
areCustomStepsContinuous
64+
customSteps={boundarySteps}
65+
locale="de-DE"
66+
onChange={onChangeDe}
67+
/>
68+
<br />
69+
<Content component="small">English locale (en-US) - uses a period as the decimal separator</Content>
70+
<Slider
71+
value={valueEn}
72+
min={0}
73+
max={1}
74+
step={0.1}
75+
isInputVisible
76+
inputValue={inputValueEn}
77+
areCustomStepsContinuous
78+
customSteps={boundarySteps}
79+
locale="en-US"
80+
onChange={onChangeEn}
81+
/>
82+
</>
83+
);
84+
};

‎packages/react-core/src/helpers/__tests__/util.test.ts‎

Lines changed: 50 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import {
33
formatLocalizedDecimal,
44
formatBreakpointMods,
55
getElementLocale,
6+
getLocalizedInputWidthChars,
67
getUniqueId,
78
debounce,
89
isElementInView,
@@ -114,18 +115,62 @@ test('formatBreakpointMods', () => {
114115
expect(formatBreakpointMods({ default: 'column', lg: 'row' }, styles)).toEqual('pf-m-column pf-m-row-on-lg');
115116
});
116117

117-
test('parseLocalizedDecimal accepts dot and comma decimal separators', () => {
118-
const enFormatter = new Intl.NumberFormat('en');
119-
const esFormatter = new Intl.NumberFormat('es');
118+
test('parseLocalizedDecimal accepts locale-formatted decimals', () => {
119+
const enFormatter = new Intl.NumberFormat('en', { useGrouping: false });
120+
const esFormatter = new Intl.NumberFormat('es', { useGrouping: false });
121+
const deFormatter = new Intl.NumberFormat('de-DE', { useGrouping: false });
120122

121123
expect(parseLocalizedDecimal('50.2', enFormatter)).toBe(50.2);
122124
expect(parseLocalizedDecimal('50,2', esFormatter)).toBe(50.2);
123-
expect(parseLocalizedDecimal('1.234,56', esFormatter)).toBe(1234.56);
124-
expect(parseLocalizedDecimal('1,234.56', enFormatter)).toBe(1234.56);
125+
expect(parseLocalizedDecimal('0,625', deFormatter)).toBe(0.625);
125126
expect(parseLocalizedDecimal('', enFormatter)).toBeNaN();
126127
});
127128

129+
test('parseLocalizedDecimal rejects mismatched separators', () => {
130+
const enFormatter = new Intl.NumberFormat('en-US', { useGrouping: false });
131+
const deFormatter = new Intl.NumberFormat('de-DE', { useGrouping: false });
132+
133+
expect(parseLocalizedDecimal('50,2', enFormatter)).toBeNaN();
134+
expect(parseLocalizedDecimal('50.2', deFormatter)).toBeNaN();
135+
expect(parseLocalizedDecimal('50,2', deFormatter)).toBe(50.2);
136+
});
137+
138+
test('parseLocalizedDecimal rejects grouping separators', () => {
139+
const enFormatter = new Intl.NumberFormat('en-US');
140+
const esFormatter = new Intl.NumberFormat('es');
141+
const deFormatter = new Intl.NumberFormat('de-DE');
142+
const enINFormatter = new Intl.NumberFormat('en-IN');
143+
144+
expect(parseLocalizedDecimal('1,234.56', enFormatter)).toBeNaN();
145+
expect(parseLocalizedDecimal('12.345,67', esFormatter)).toBeNaN();
146+
expect(parseLocalizedDecimal('1.234,56', deFormatter)).toBeNaN();
147+
expect(parseLocalizedDecimal('12,34,567', enINFormatter)).toBeNaN();
148+
});
149+
150+
test('parseLocalizedDecimal rejects malformed numeric tokens', () => {
151+
const enFormatter = new Intl.NumberFormat('en-US');
152+
153+
expect(parseLocalizedDecimal('12abc', enFormatter)).toBeNaN();
154+
expect(parseLocalizedDecimal('1,2abc', enFormatter)).toBeNaN();
155+
expect(parseLocalizedDecimal('12-3', enFormatter)).toBeNaN();
156+
expect(parseLocalizedDecimal('--12', enFormatter)).toBeNaN();
157+
expect(parseLocalizedDecimal('+-12', enFormatter)).toBeNaN();
158+
expect(parseLocalizedDecimal('12.3.4', enFormatter)).toBeNaN();
159+
expect(parseLocalizedDecimal('abc', enFormatter)).toBeNaN();
160+
161+
expect(parseLocalizedDecimal('-50.2', enFormatter)).toBe(-50.2);
162+
expect(parseLocalizedDecimal('+12', enFormatter)).toBe(12);
163+
expect(parseLocalizedDecimal('1,234.56', enFormatter)).toBeNaN();
164+
});
165+
128166
test('formatLocalizedDecimal uses locale decimal separator', () => {
129167
expect(formatLocalizedDecimal(50.2, new Intl.NumberFormat('en-US'))).toBe('50.2');
130168
expect(formatLocalizedDecimal(50.2, new Intl.NumberFormat('de-DE'))).toBe('50,2');
131169
});
170+
171+
test('getLocalizedInputWidthChars stays wide enough for shorter formatted values', () => {
172+
const formatter = new Intl.NumberFormat('de-DE', { useGrouping: false });
173+
174+
expect(getLocalizedInputWidthChars(formatter, 0, 1, '1')).toBe(formatLocalizedDecimal(0.99, formatter).length);
175+
expect(getLocalizedInputWidthChars(formatter, 0, 100, '5')).toBe(formatLocalizedDecimal(100, formatter).length);
176+
});

0 commit comments

Comments
 (0)