feat(Slider): Recognize localized number input - #12599
Open
rebeccaalpert wants to merge 2 commits into
Open
Conversation
WalkthroughSlider value inputs now support locale-aware decimal display and parsing through ChangesLocalized Slider Input
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SliderInput
participant Slider
participant parseLocalizedDecimal
participant formatLocalizedDecimal
participant ParentOnChange
SliderInput->>Slider: Enter localized decimal
Slider->>parseLocalizedDecimal: Parse value with Intl.NumberFormat
parseLocalizedDecimal-->>Slider: Return numeric value
Slider->>ParentOnChange: Report numeric value and display updater
Slider->>formatLocalizedDecimal: Format synchronized value
formatLocalizedDecimal-->>SliderInput: Return localized display value
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
rebeccaalpert
force-pushed
the
slider
branch
7 times, most recently
from
July 28, 2026 14:58
111a2a9 to
f5ccd76
Compare
rebeccaalpert
marked this pull request as ready for review
July 28, 2026 15:13
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/react-core/src/components/Slider/Slider.tsx`:
- Around line 131-140: Initialize inputDisplayValue in the Slider component
using the locale prop instead of the hardcoded 'en' locale, so the first render
is formatted consistently with the requested locale and avoids an incorrect
initial display.
- Around line 170-183: The functional updater inside
setLocalInputValueWithDisplay must remain pure. Resolve and apply the numeric
state through setLocalInputValue, then move updateInputDisplay outside that
updater into a post-commit effect or separate update while preserving the
isInputFocusedRef behavior.
- Around line 152-190: Update the updateInputDisplay useCallback dependency
array to include locale, ensuring formatting reflects runtime locale changes
while preserving all existing callers and behavior.
In `@packages/react-core/src/helpers/util.ts`:
- Around line 602-611: Update parseLocalizedDecimal to wrap
Intl.NumberFormat(locale) construction and subsequent locale-based parsing setup
in a try/catch. On invalid locale input, fall back to Intl.NumberFormat() and
continue using its group and decimal symbols so Slider render/input callbacks do
not throw.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ea4875c7-d8e4-4842-8ece-4db9acfaa07f
⛔ Files ignored due to path filters (1)
packages/react-core/src/components/Slider/__tests__/__snapshots__/Slider.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
packages/react-core/src/components/Slider/Slider.tsxpackages/react-core/src/components/Slider/__tests__/Slider.test.tsxpackages/react-core/src/helpers/__tests__/util.test.tspackages/react-core/src/helpers/util.tspackages/tsconfig.base.json
Assisted-by: Cursor Fixes patternfly#12052
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/react-core/src/helpers/util.ts`:
- Around line 627-632: Update the numeric parsing logic around the cleaned value
and parseFloat so it validates the entire normalized input before conversion,
returning NaN for malformed tokens such as embedded letters, misplaced
separators, or multiple signs. Preserve valid numeric formats and ensure Slider
blur handling only commits fully valid values.
- Around line 607-630: Update the number normalization flow around groupSymbol,
decimalSymbol, and cleaned so grouping removal is only applied when
formatter.formatToParts reports an actual group separator, never by falling back
to the decimal separator. Before replacing the locale decimal symbol with '.',
reject inputs containing alternate grouping characters that would otherwise be
silently merged, preserving valid locale-formatted values while making cases
such as comma-decimal en-US and mixed-separator de-DE inputs invalid.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 86eb8ae5-f520-44c6-9103-d0319c3c8b15
⛔ Files ignored due to path filters (1)
packages/react-core/src/components/Slider/__tests__/__snapshots__/Slider.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
packages/react-core/src/components/Slider/Slider.tsxpackages/react-core/src/components/Slider/__tests__/Slider.test.tsxpackages/react-core/src/helpers/__tests__/util.test.tspackages/react-core/src/helpers/util.tspackages/tsconfig.base.json
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/tsconfig.base.json
- packages/react-core/src/helpers/tests/util.test.ts
- packages/react-core/src/components/Slider/tests/Slider.test.tsx
- packages/react-core/src/components/Slider/Slider.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react-core/src/helpers/util.ts (1)
653-661: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftHardcoded 3-digit grouping breaks Indian-numbering locales (en-IN, hi-IN, etc.).
The grouping-structure check assumes every group after the first is exactly 3 digits (
/^\d{3}$/). This holds for Western grouping but not for the Indian numbering system, whichIntl.NumberFormat('en-IN')(and similar locales) uses: e.g.1234567.89formats as"12,34,567.89"— groups of 2 digits after the initial 3.Tracing the current code with an
en-INformatter on'12,34,567.89':intPart.split(',')→['12','34','567'];groups.slice(1)=['34','567'], and'34'fails/^\d{3}$/, so the function returnsNaNfor a value that the very same formatter would itself produce via.format(). This breaks slider/input synchronization for any consumer using an Indian-numbering locale with values ≥ 1 lakh (100,000) — directly undermining this PR's goal of general non-English locale support.A more robust check would validate grouping structure against the formatter's own output (e.g., compare the input's grouping to what
formatter.format()produces for the parsed integer) rather than hardcoding a single Western pattern. Happy to help draft this if useful.new Intl.NumberFormat("en-IN").format(1234567.89); // "12,34,567.89" — not X,XXX,XXX🌐 Web verification
Confirmed via MDN/W3C: const enINFormatter = new Intl.NumberFormat('en-IN'); console.log(
Indian English: ${enINFormatter.format(number)}); // Output: Indian English: 12,34,567.89🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-core/src/helpers/util.ts` around lines 653 - 661, Update the grouping validation in the integer-parsing logic around intPart and groupSymbol to validate against the configured Intl.NumberFormat output for the parsed integer, rather than requiring every trailing group to contain three digits. Preserve rejection of malformed or silently mergeable grouping, while accepting valid Indian-style formats such as en-IN’s “12,34,567”.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/react-core/src/helpers/util.ts`:
- Around line 653-661: Update the grouping validation in the integer-parsing
logic around intPart and groupSymbol to validate against the configured
Intl.NumberFormat output for the parsed integer, rather than requiring every
trailing group to contain three digits. Preserve rejection of malformed or
silently mergeable grouping, while accepting valid Indian-style formats such as
en-IN’s “12,34,567”.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2abcc166-fb42-471e-a5cc-cca98a928e53
📒 Files selected for processing (2)
packages/react-core/src/helpers/__tests__/util.test.tspackages/react-core/src/helpers/util.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-core/src/helpers/tests/util.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/react-core/src/helpers/util.ts`:
- Around line 612-650: Update parseLocalizedDecimal to normalize locale-specific
numeral glyphs to ASCII before validation, using a digit map derived from
formatter.formatToParts. Apply this normalization to the trimmed input and to
the grouped-format comparison, while preserving the existing separator,
grouping, sign, and fractional validation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5881202d-1816-4d60-9b2b-f37297d4d571
📒 Files selected for processing (2)
packages/react-core/src/helpers/__tests__/util.test.tspackages/react-core/src/helpers/util.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-core/src/helpers/tests/util.test.ts
Comment on lines
+612
to
+650
| const normalizedString = value.trim(); | ||
|
|
||
| // Reject '.' / ',' when they are neither this locale's decimal nor its reported group separator | ||
| // (e.g. mixed-separator de-DE input like "1,234.56"). | ||
| for (const sep of ['.', ','] as const) { | ||
| if (sep !== decimalSymbol && sep !== groupSymbol && normalizedString.includes(sep)) { | ||
| return NaN; | ||
| } | ||
| } | ||
|
|
||
| const decimalIndex = normalizedString.indexOf(decimalSymbol); | ||
| if (decimalIndex !== -1 && normalizedString.indexOf(decimalSymbol, decimalIndex + decimalSymbol.length) !== -1) { | ||
| return NaN; | ||
| } | ||
|
|
||
| let intPart = decimalIndex === -1 ? normalizedString : normalizedString.slice(0, decimalIndex); | ||
| const fracPart = decimalIndex === -1 ? undefined : normalizedString.slice(decimalIndex + decimalSymbol.length); | ||
|
|
||
| let sign = ''; | ||
| if (intPart.startsWith('-') || intPart.startsWith('+')) { | ||
| sign = intPart[0] === '-' ? '-' : ''; | ||
| intPart = intPart.slice(1); | ||
| } | ||
|
|
||
| if (fracPart !== undefined && groupSymbol && fracPart.includes(groupSymbol)) { | ||
| return NaN; | ||
| } | ||
|
|
||
| // If grouping appears, require locale-valid groups so values like en-US "50,2" are invalid | ||
| // instead of silently merging into 502, while accepting locales such as en-IN "12,34,567". | ||
| if (groupSymbol && intPart.includes(groupSymbol)) { | ||
| const digitsOnly = intPart.split(groupSymbol).join(''); | ||
| if (!/^\d+$/.test(digitsOnly) || formatter.format(Number(digitsOnly)) !== intPart) { | ||
| return NaN; | ||
| } | ||
| intPart = digitsOnly; | ||
| } | ||
|
|
||
| if (!/^\d*$/.test(intPart) || (fracPart !== undefined && !/^\d*$/.test(fracPart))) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
node <<'JS'
const formatter = new Intl.NumberFormat('ar-EG');
console.log(formatter.format(50.2));
console.log(formatter.formatToParts(1234567890));
JSRepository: patternfly/patternfly-react
Length of output: 418
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the helper implementation and relevant usages without running repository code.
fd -a 'util\.ts$' packages/react-core/src/helpers 2>/dev/null | sed 's#^\./##'
printf '\n--- util.ts outline around parseLocalizedDecimal/formatLocalizedDecimal ---\n'
ast-grep outline packages/react-core/src/helpers/util.ts --match parseLocalizedDecimal --view expanded || true
printf '\n--- relevant source lines ---\n'
sed -n '560,680p' packages/react-core/src/helpers/util.ts
printf '\n--- relevant tests/usages ---\n'
rg -n "parseLocalizedDecimal|formatLocalizedDecimal|ar-EG|٥٠٫٢|formattedDigits|localized" packages -g '!**/node_modules/**' || trueRepository: patternfly/patternfly-react
Length of output: 10749
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node <<'JS'
const locales = ['ar-EG', 'ar-SA', 'fa-IR', 'hi-IN', 'bn-IN', 'de-DE', 'en-IN'];
for (const locale of locales) {
const f = new Intl.NumberFormat(locale);
console.log(locale, 'format(500.2):', f.format(500.2));
const parts = f.formatToParts(1234567890);
const locals = parts.filter(p => p.type === 'integer').map(p => p.value).join('');
console.log(' integer glyphs for 1234567890:', locals);
}
JS
printf '\n--- Slider formatter construction ---\n'
sed -n '1,190p' packages/react-core/src/components/Slider/Slider.tsxRepository: patternfly/patternfly-react
Length of output: 7846
Normalize locale-specific numeral glyphs before validating.
formatLocalizedDecimal can return values like ar-EG ٥٠٫٢, but parseLocalizedDecimal only accepts ASCII digits, so the displayed value roundtrips to NaN. Build an ASCII-normalization map from formatToParts, normalize both the input and the grouped formatting comparison, then validate.
Proposed fix
- const normalizedString = value.trim();
+ const localDigits = formatter
+ .formatToParts(1234567890)
+ .filter((part) => part.type === 'integer')
+ .map((part) => part.value)
+ .join('');
+ const digitMap = new Map([...localDigits].map((digit, index) => [digit, '1234567890'[index]]));
+ const normalizeDigits = (input: string) => [...input].map((char) => digitMap.get(char) || char).join('');
+ const normalizedString = normalizeDigits(value.trim());
...
- if (!/^\d+$/.test(digitsOnly) || formatter.format(Number(digitsOnly)) !== intPart) {
+ if (!/^\d+$/.test(digitsOnly) || normalizeDigits(formatter.format(Number(digitsOnly))) !== intPart) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const normalizedString = value.trim(); | |
| // Reject '.' / ',' when they are neither this locale's decimal nor its reported group separator | |
| // (e.g. mixed-separator de-DE input like "1,234.56"). | |
| for (const sep of ['.', ','] as const) { | |
| if (sep !== decimalSymbol && sep !== groupSymbol && normalizedString.includes(sep)) { | |
| return NaN; | |
| } | |
| } | |
| const decimalIndex = normalizedString.indexOf(decimalSymbol); | |
| if (decimalIndex !== -1 && normalizedString.indexOf(decimalSymbol, decimalIndex + decimalSymbol.length) !== -1) { | |
| return NaN; | |
| } | |
| let intPart = decimalIndex === -1 ? normalizedString : normalizedString.slice(0, decimalIndex); | |
| const fracPart = decimalIndex === -1 ? undefined : normalizedString.slice(decimalIndex + decimalSymbol.length); | |
| let sign = ''; | |
| if (intPart.startsWith('-') || intPart.startsWith('+')) { | |
| sign = intPart[0] === '-' ? '-' : ''; | |
| intPart = intPart.slice(1); | |
| } | |
| if (fracPart !== undefined && groupSymbol && fracPart.includes(groupSymbol)) { | |
| return NaN; | |
| } | |
| // If grouping appears, require locale-valid groups so values like en-US "50,2" are invalid | |
| // instead of silently merging into 502, while accepting locales such as en-IN "12,34,567". | |
| if (groupSymbol && intPart.includes(groupSymbol)) { | |
| const digitsOnly = intPart.split(groupSymbol).join(''); | |
| if (!/^\d+$/.test(digitsOnly) || formatter.format(Number(digitsOnly)) !== intPart) { | |
| return NaN; | |
| } | |
| intPart = digitsOnly; | |
| } | |
| if (!/^\d*$/.test(intPart) || (fracPart !== undefined && !/^\d*$/.test(fracPart))) { | |
| const localDigits = formatter | |
| .formatToParts(1234567890) | |
| .filter((part) => part.type === 'integer') | |
| .map((part) => part.value) | |
| .join(''); | |
| const digitMap = new Map([...localDigits].map((digit, index) => [digit, '1234567890'[index]])); | |
| const normalizeDigits = (input: string) => [...input].map((char) => digitMap.get(char) || char).join(''); | |
| const normalizedString = normalizeDigits(value.trim()); | |
| // Reject '.' / ',' when they are neither this locale's decimal nor its reported group separator | |
| // (e.g. mixed-separator de-DE input like "1,234.56"). | |
| for (const sep of ['.', ','] as const) { | |
| if (sep !== decimalSymbol && sep !== groupSymbol && normalizedString.includes(sep)) { | |
| return NaN; | |
| } | |
| } | |
| const decimalIndex = normalizedString.indexOf(decimalSymbol); | |
| if (decimalIndex !== -1 && normalizedString.indexOf(decimalSymbol, decimalIndex + decimalSymbol.length) !== -1) { | |
| return NaN; | |
| } | |
| let intPart = decimalIndex === -1 ? normalizedString : normalizedString.slice(0, decimalIndex); | |
| const fracPart = decimalIndex === -1 ? undefined : normalizedString.slice(decimalIndex + decimalSymbol.length); | |
| let sign = ''; | |
| if (intPart.startsWith('-') || intPart.startsWith('+')) { | |
| sign = intPart[0] === '-' ? '-' : ''; | |
| intPart = intPart.slice(1); | |
| } | |
| if (fracPart !== undefined && groupSymbol && fracPart.includes(groupSymbol)) { | |
| return NaN; | |
| } | |
| // If grouping appears, require locale-valid groups so values like en-US "50,2" are invalid | |
| // instead of silently merging into 502, while accepting locales such as en-IN "12,34,567". | |
| if (groupSymbol && intPart.includes(groupSymbol)) { | |
| const digitsOnly = intPart.split(groupSymbol).join(''); | |
| if (!/^\d+$/.test(digitsOnly) || normalizeDigits(formatter.format(Number(digitsOnly))) !== intPart) { | |
| return NaN; | |
| } | |
| intPart = digitsOnly; | |
| } | |
| if (!/^\d*$/.test(intPart) || (fracPart !== undefined && !/^\d*$/.test(fracPart))) { |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/react-core/src/helpers/util.ts` around lines 612 - 650, Update
parseLocalizedDecimal to normalize locale-specific numeral glyphs to ASCII
before validation, using a digit map derived from formatter.formatToParts. Apply
this normalization to the trimmed input and to the grouped-format comparison,
while preserving the existing separator, grouping, sign, and fractional
validation behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I went with a locale prop since it seemed like it would provide a better, more consistent solution. There are a lot of ways people could be setting language. Examples: https://github.com/i18next/i18next-browser-languageDetector#introduction.
OpenShift (the internationalized system I'm most familiar with) doesn't update the lang attribute when it changes languages, so I didn't want to rely on that.
What: Closes #12052
Additional issues:
Summary by CodeRabbit
es2018.intl.