Add snapOnStep, smallStep support for NumberField #1601
Conversation
🦋 Changeset detectedLatest commit: a4ff45f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughAdds locale to NumberField context/props, introduces smallStep and snapOnStep with defaults, adds a 'normal' step type, updates input/key/button handlers to use 'normal' by default (Alt for 'small'), and implements decimal-aware stepping with optional snapping and clamping. Changes
Sequence Diagram(s)sequenceDiagram autonumber actor User participant Decr as Decrement Button participant Inc as Increment Button participant Input as Input Field participant Root as NumberFieldRoot participant Context as NumberFieldContext/State rect rgb(242,248,255) User->>Decr: Click Decr->>Root: handleStep(type:'normal', direction:'decrement') end rect rgb(245,255,245) User->>Inc: Click Inc->>Root: handleStep(type:'normal', direction:'increment') end rect rgb(255,250,240) User->>Input: Key ArrowUp/ArrowDown alt Alt pressed Input->>Root: handleStep(type:'small', direction:inc/dec) else no modifier Input->>Root: handleStep(type:'normal', direction:inc/dec) end end Root->>Root: applyStep(amount)\n- init empty -> min or 0\n- optional snap (large)\n- decimal-aware calc (scale/round/unscale)\n- clamp to min/max Root->>Context: set value(next) Context-->>Input: re-render with value (and locale) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/components/ui/NumberField/contexts/NumberFieldContext.tsx (1)
3-7: Expand handleStep type to include 'normal' to resolve TS errors.This aligns the context with Root and all call sites (clicks/keys).
Apply this diff:
export type NumberFieldContextType = { inputValue: number|''; handleOnChange: (input: number|'') => void; - handleStep: (opts: { direction: 'increment' | 'decrement'; type: 'small' | 'large' }) => void; + handleStep: (opts: { direction: 'increment' | 'decrement'; type: 'small' | 'normal' | 'large' }) => void; id?: string; name?: string; disabled?: boolean;src/components/ui/NumberField/fragments/NumberFieldInput.tsx (2)
27-52: Prevent double-stepping when Alt+Shift is pressed; simplify key handling.Multiple independent ifs can fire (e.g., Alt+Shift). Use a single conditional chain with precedence: Shift > Alt > default.
Apply this diff:
- const handleKeyDown = (event: React.KeyboardEvent<HTMLInputElement>) => { - if (event.key === 'ArrowUp' && !event.shiftKey) { - event.preventDefault(); - handleStep({ direction: 'increment', type: 'normal' }); - } - if (event.key === 'ArrowDown' && !event.shiftKey) { - event.preventDefault(); - handleStep({ direction: 'decrement', type: 'normal' }); - } - if (event.key === 'ArrowUp' && event.shiftKey) { - event.preventDefault(); - handleStep({ direction: 'increment', type: 'large' }); - } - if (event.key === 'ArrowDown' && event.shiftKey) { - event.preventDefault(); - handleStep({ direction: 'decrement', type: 'large' }); - } - if (event.key === 'ArrowUp' && event.altKey) { - event.preventDefault(); - handleStep({ direction: 'increment', type: 'small' }); - } - if (event.key === 'ArrowDown' && event.altKey) { - event.preventDefault(); - handleStep({ direction: 'decrement', type: 'small' }); - } - }; + const handleKeyDown = (event: React.KeyboardEvent<HTMLInputElement>) => { + const isUp = event.key === 'ArrowUp'; + const isDown = event.key === 'ArrowDown'; + if (!isUp && !isDown) return; + event.preventDefault(); + const direction = isUp ? 'increment' : 'decrement'; + if (event.shiftKey) { + handleStep({ direction, type: 'large' }); + } else if (event.altKey) { + handleStep({ direction, type: 'small' }); + } else { + handleStep({ direction, type: 'normal' }); + } + };
55-66: Locale parsing/formatting not implemented (type="number" can’t accept locale formats).To satisfy the locale requirement, input should handle localized strings via Intl.NumberFormat and parsing, typically with type="text", inputMode="decimal", and formatted display on blur. Current Number(val) ignores locale and may yield NaN or reject localized input.
I can add a formatter/parser with caret-safe editing (display: formatted on blur; internal state: number | ''), plus acceptance of thousand separators and locale decimal. Proceed?
🧹 Nitpick comments (4)
src/components/ui/NumberField/stories/NumberField.stories.tsx (1)
24-24: Avoid mixing controlled and defaultValue; optionally demo locale prop.In a controlled story, remove defaultValue to prevent ambiguity; optionally add locale to showcase the new API.
Apply this diff:
- <NumberField.Root value={value} onValueChange={setValue} defaultValue={3} step={1} snapOnStep={true} largeStep={5}> + <NumberField.Root value={value} onValueChange={setValue} step={1} snapOnStep={true} largeStep={5} /* locale="de-DE" */>src/components/ui/NumberField/fragments/NumberFieldInput.tsx (1)
58-60: Consider sanitizing NaN onChange.Number(val) can yield NaN; handleOnChange('') or ignore update in that case to avoid NaN state.
- onChange={(e) => { const val = e.target.value; handleOnChange(val === '' ? '' : Number(val)); }} + onChange={(e) => { + const val = e.target.value; + if (val === '') return handleOnChange(''); + const n = Number(val); + if (Number.isNaN(n)) return; + handleOnChange(n); + }}src/components/ui/NumberField/fragments/NumberFieldRoot.tsx (2)
105-107: Remove console.log from decrement path.Leftover debug logging.
Apply this diff:
if (direction === 'decrement') { amount = -amount; - console.log(amount); }
57-61: Minor: prev cannot be null.Type is number | ''. The null check is redundant.
- if (prev === '' || prev === null) { + if (prev === '') { temp = min !== undefined ? min : 0; } else { temp = Number(prev); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/components/ui/NumberField/contexts/NumberFieldContext.tsx(1 hunks)src/components/ui/NumberField/fragments/NumberFieldDecrement.tsx(1 hunks)src/components/ui/NumberField/fragments/NumberFieldIncrement.tsx(1 hunks)src/components/ui/NumberField/fragments/NumberFieldInput.tsx(2 hunks)src/components/ui/NumberField/fragments/NumberFieldRoot.tsx(3 hunks)src/components/ui/NumberField/stories/NumberField.stories.tsx(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Tests
src/components/ui/NumberField/fragments/NumberFieldInput.tsx
[error] 30-34: Type '"normal"' is not assignable to type '"small" | "large"'. TS2322 in handleStep calls within NumberFieldInput.tsx. This causes test suite failures (NumberField tests).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: coverage
🔇 Additional comments (4)
src/components/ui/NumberField/fragments/NumberFieldDecrement.tsx (1)
20-21: Button disabled state respects disabled/readOnly (LGTM).Good accessibility safeguard to prevent interactions.
src/components/ui/NumberField/fragments/NumberFieldIncrement.tsx (1)
20-21: Button disabled state respects disabled/readOnly (LGTM).Matches Decrement.
src/components/ui/NumberField/fragments/NumberFieldRoot.tsx (2)
86-104: API alignment looks good.Adding 'normal' alongside small/large with sensible defaults is coherent.
Please confirm step=0 is not a supported config (guard returns early), and that largeStep/smallStep > 0 are validated as well.
27-27: Prop defaults and new API (smallStep, snapOnStep, locale) look sane.Good defaults: largeStep=10, step=1, smallStep=0.1, snapOnStep=false.
| locale?: string; | ||
| }; |
There was a problem hiding this comment.
Locale added to context but unused across components.
The PR goal includes locale formatting/parsing. Passing locale without using Intl.NumberFormat for display/parse doesn’t meet the feature.
I can provide a minimal formatter/parser integration (Intl.NumberFormat + inputMode="decimal" + text input with onBlur formatting and onChange parsing). Want a patch?
🤖 Prompt for AI Agents
In src/components/ui/NumberField/contexts/NumberFieldContext.tsx around lines 13-14, the locale property is being passed in the context but never used for formatting/parsing; update the NumberField implementation to create an Intl.NumberFormat from that locale and use it to format the displayed value on blur and to parse user input on change. Specifically: derive a formatter = new Intl.NumberFormat(locale, { maximumFractionDigits, useGrouping: true }) and use inputMode="decimal" on the input; onChange keep a raw string state and parse it by stripping grouping chars and replacing the locale decimal separator with '.' then Number(parsed) (handle empty string and NaN), and onBlur replace the input value with formatter.format(parsedNumber); ensure the context exposes locale or the formatter so child components use the same formatter, and provide sensible fallbacks when locale is missing. | <button | ||
| ref={ref} | ||
| onClick={() => handleStep({ direction: 'decrement', type: 'small' })} | ||
| onClick={() => handleStep({ direction: 'decrement', type: 'normal' })} |
There was a problem hiding this comment.
"normal" step type will not compile until context type is updated.
handleStep is invoked with type: 'normal', but NumberFieldContext.handleStep only allows 'small' | 'large'. This will cause TS errors until the context type includes 'normal'.
Fix is in NumberFieldContext.tsx (see suggested diff there).
🤖 Prompt for AI Agents
In src/components/ui/NumberField/fragments/NumberFieldDecrement.tsx around line 18, the onClick calls handleStep with type: 'normal' but NumberFieldContext.handleStep currently only accepts 'small' | 'large'; update the context type definition in src/components/ui/NumberField/NumberFieldContext.tsx to include 'normal' in the union and ensure the context implementation and any switch/handlers accept and properly handle the 'normal' case (or alternatively change this call to an allowed type), then rebuild to confirm TypeScript errors are resolved. | <button | ||
| ref={ref} | ||
| onClick={() => handleStep({ direction: 'increment', type: 'small' })} | ||
| onClick={() => handleStep({ direction: 'increment', type: 'normal' })} |
There was a problem hiding this comment.
Same type mismatch as Decrement ("normal" not in union).
handleStep({ type: 'normal' }) requires NumberFieldContext to allow 'normal'.
See the context type fix suggested in NumberFieldContext.tsx.
🤖 Prompt for AI Agents
In src/components/ui/NumberField/fragments/NumberFieldIncrement.tsx at line 19, the call handleStep({ direction: 'increment', type: 'normal' }) uses a 'normal' type that isn't part of the StepType union; update the NumberFieldContext type definition (in NumberFieldContext.tsx) to include 'normal' in the union for the step type (and any related types/interfaces), or alternatively change the call here to one of the existing allowed type values — prefer adding 'normal' to the context type so handleStep accepts { type: 'normal' } consistently across components. | if (temp % largeStep != 0 && snapOnStep && largeStep === Math.abs(amount)) { temp = Math.round(temp / largeStep) * largeStep; } | ||
| | ||
| // Find decimal places in amount only | ||
| const amountDecimals = (amount.toString().split('.')[1] || '').length; | ||
| if (amountDecimals === 0) { | ||
| nextValue = temp + amount; | ||
| } | ||
| else{ | ||
| const factor = Math.pow(10, amountDecimals); | ||
| | ||
| if (min !== undefined && nextValue < min) { | ||
| return min; | ||
| // Scale value to integer, apply step, then scale back | ||
| let scaledValue = Math.round(temp * factor) + Math.round(amount * factor); | ||
| nextValue = scaledValue / factor; | ||
| } |
There was a problem hiding this comment.
snapOnStep only snaps for largeStep and uses modulo on floats; generalize and make decimal-safe.
Spec calls for snapping to step increments. Snap using the active step size (abs(amount)), origin at min (if provided) or 0, and avoid float modulo.
Apply this diff:
- if (temp % largeStep != 0 && snapOnStep && largeStep === Math.abs(amount)) { temp = Math.round(temp / largeStep) * largeStep; } + const stepSize = Math.abs(amount); + if (snapOnStep && stepSize > 0) { + const origin = min !== undefined ? min : 0; + const stepDecimals = (stepSize.toString().split('.')[1] || '').length; + const f = Math.pow(10, stepDecimals); + const tempInt = Math.round((temp - origin) * f); + const stepInt = Math.round(stepSize * f); + temp = origin + Math.round(tempInt / stepInt) * stepInt / f; + } - // Find decimal places in amount only - const amountDecimals = (amount.toString().split('.')[1] || '').length; - if (amountDecimals === 0) { - nextValue = temp + amount; - } - else{ - const factor = Math.pow(10, amountDecimals); - - // Scale value to integer, apply step, then scale back - let scaledValue = Math.round(temp * factor) + Math.round(amount * factor); - nextValue = scaledValue / factor; - } + // Find decimal places in amount only + const amountDecimals = (amount.toString().split('.')[1] || '').length; + if (amountDecimals === 0) { + nextValue = temp + amount; + } else { + const factor = Math.pow(10, amountDecimals); + // Apply step then round once to the target precision + nextValue = Math.round((temp + amount) * factor) / factor; + }📝 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.
| if (temp % largeStep != 0 && snapOnStep && largeStep === Math.abs(amount)) { temp = Math.round(temp / largeStep) * largeStep; } | |
| // Find decimal places in amount only | |
| const amountDecimals = (amount.toString().split('.')[1] || '').length; | |
| if (amountDecimals === 0) { | |
| nextValue = temp + amount; | |
| } | |
| else{ | |
| const factor = Math.pow(10, amountDecimals); | |
| if (min !== undefined && nextValue < min) { | |
| return min; | |
| // Scale value to integer, apply step, then scale back | |
| let scaledValue = Math.round(temp * factor) + Math.round(amount * factor); | |
| nextValue = scaledValue / factor; | |
| } | |
| const stepSize = Math.abs(amount); | |
| if (snapOnStep && stepSize > 0) { | |
| const origin = min !== undefined ? min : 0; | |
| const stepDecimals = (stepSize.toString().split('.')[1] || '').length; | |
| const f = Math.pow(10, stepDecimals); | |
| const tempInt = Math.round((temp - origin) * f); | |
| const stepInt = Math.round(stepSize * f); | |
| temp = origin + Math.round(tempInt / stepInt) * stepInt / f; | |
| } | |
| // Find decimal places in amount only | |
| const amountDecimals = (amount.toString().split('.')[1] || '').length; | |
| if (amountDecimals === 0) { | |
| nextValue = temp + amount; | |
| } else { | |
| const factor = Math.pow(10, amountDecimals); | |
| // Apply step then round once to the target precision | |
| nextValue = Math.round((temp + amount) * factor) / factor; | |
| } |
🤖 Prompt for AI Agents
In src/components/ui/NumberField/fragments/NumberFieldRoot.tsx around lines 63 to 76, the current code only snaps when largeStep matches abs(amount) and uses float modulo; change to always snap using the active step size (const step = Math.abs(amount)) with origin = typeof min === 'number' ? min : 0, and perform snapping using integer math: compute decimalPlaces = (step.toString().split('.')[1] || '').length or compute factor = Math.pow(10, decimalPlaces or max decimal places between step and temp origin), scale offset = Math.round((temp - origin) * factor) and stepInt = Math.round(step * factor), then if (snapOnStep) compute remainder = offset % stepInt and adjust offset by Math.round(remainder / stepInt) * stepInt (or subtract remainder to snap to nearest multiple), finally set nextValue = origin + offset / factor; this avoids float modulo and generalizes snapping to the active step with origin at min or 0. 
GoldGroove06 changed the title GoldGroove06 changed the title There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/components/ui/NumberField/fragments/NumberFieldRoot.tsx (1)
63-66: snapOnStep only snaps for largeStep and uses float modulo; needs generalization.The current snapping logic has several issues:
- Uses float modulo (
temp % largeStep) which suffers from floating-point precision errors- Only snaps when
largeStep === Math.abs(amount), ignoringstepandsmallStep- Should snap to the active step size for any increment type
Per the past review, snap using the active step size with origin at
min(or 0) using integer math to avoid float precision issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/ui/NumberField/contexts/NumberFieldContext.tsx(1 hunks)src/components/ui/NumberField/fragments/NumberFieldRoot.tsx(3 hunks)src/components/ui/NumberField/stories/NumberField.stories.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/ui/NumberField/contexts/NumberFieldContext.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/ui/NumberField/fragments/NumberFieldRoot.tsx (1)
src/core/hooks/useControllableState/index.tsx (1)
useControllableState(15-78)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build
- GitHub Check: coverage
🔇 Additional comments (5)
src/components/ui/NumberField/fragments/NumberFieldRoot.tsx (4)
17-19: LGTM! New props align with PR objectives.The three new optional props (smallStep, snapOnStep, locale) are correctly typed and match the feature requirements from issue #1599.
27-27: LGTM! Sensible defaults for new props.The default values (largeStep=10, step=1, smallStep=0.1, snapOnStep=false) are appropriate and follow a logical progression for stepping granularity.
34-50: LGTM! Standard clamping logic.The handleOnChange function correctly handles empty inputs and enforces min/max boundaries.
114-125: LGTM! Locale correctly added to context.The locale prop is properly propagated through the context to enable downstream components to access it.
src/components/ui/NumberField/stories/NumberField.stories.tsx (1)
24-24: Verify removal of min/max props is intentional.The Controlled story no longer specifies
minormaxprops, while the Basic story retains them. If this is to demonstrate unbounded stepping behavior, consider adding a comment to clarify the intent. Otherwise, verify this wasn't an accidental omission.
| const amountDecimals = (amount.toString().split('.')[1] || '').length; | ||
| const tempDecimals = (temp.toString().split('.')[1] || '').length; | ||
| const decimal = amountDecimals > tempDecimals ? amountDecimals : tempDecimals; | ||
| | ||
| // multiply temp and amount with same decimal count to remove decimal places | ||
| nextValue = (temp * Math.pow(10, decimal)) + (amount * Math.pow(10, decimal)); | ||
| | ||
| // divide nextValue by same decimal count | ||
| nextValue = nextValue / Math.pow(10, decimal); | ||
| const factor = Math.pow(10, decimal); | ||
| nextValue = (Math.round(temp * factor) + Math.round(amount * factor)) / factor; |
There was a problem hiding this comment.
Remove dead code and fix decimal handling.
Lines 73-76 compute nextValue but are immediately overwritten by lines 77-78, making the first calculation dead code. Additionally, the approach is inconsistent:
- Lines 73-76 scale, add, then descale without rounding intermediate results
- Lines 77-78 round each operand before adding (better for precision)
Apply this diff to remove dead code and clarify the logic:
- const amountDecimals = (amount.toString().split('.')[1] || '').length; - const tempDecimals = (temp.toString().split('.')[1] || '').length; - const decimal = amountDecimals > tempDecimals ? amountDecimals : tempDecimals; - - // multiply temp and amount with same decimal count to remove decimal places - nextValue = (temp * Math.pow(10, decimal)) + (amount * Math.pow(10, decimal)); - - // divide nextValue by same decimal count - nextValue = nextValue / Math.pow(10, decimal); + // Find decimal places in amount and temp + const amountDecimals = (amount.toString().split('.')[1] || '').length; + const tempDecimals = (temp.toString().split('.')[1] || '').length; + const decimal = Math.max(amountDecimals, tempDecimals); + + // Scale to integers, compute step, then scale back const factor = Math.pow(10, decimal); nextValue = (Math.round(temp * factor) + Math.round(amount * factor)) / factor;📝 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.
| const amountDecimals = (amount.toString().split('.')[1] || '').length; | |
| const tempDecimals = (temp.toString().split('.')[1] || '').length; | |
| const decimal = amountDecimals > tempDecimals ? amountDecimals : tempDecimals; | |
| // multiply temp and amount with same decimal count to remove decimal places | |
| nextValue = (temp * Math.pow(10, decimal)) + (amount * Math.pow(10, decimal)); | |
| // divide nextValue by same decimal count | |
| nextValue = nextValue / Math.pow(10, decimal); | |
| const factor = Math.pow(10, decimal); | |
| nextValue = (Math.round(temp * factor) + Math.round(amount * factor)) / factor; | |
| // Find decimal places in amount and temp | |
| const amountDecimals = (amount.toString().split('.')[1] || '').length; | |
| const tempDecimals = (temp.toString().split('.')[1] || '').length; | |
| const decimal = Math.max(amountDecimals, tempDecimals); | |
| // Scale to integers, compute step, then scale back | |
| const factor = Math.pow(10, decimal); | |
| nextValue = (Math.round(temp * factor) + Math.round(amount * factor)) / factor; |
🤖 Prompt for AI Agents
In src/components/ui/NumberField/fragments/NumberFieldRoot.tsx around lines 68 to 78, remove the dead scaling/descaling block (the computation that assigns nextValue using temp * Math.pow(10, decimal) and then divides it back) and keep a single, consistent calculation that uses the computed decimal (max of amount and temp decimals), a factor = Math.pow(10, decimal), and rounds each operand before adding: nextValue should be computed as (Math.round(temp * factor) + Math.round(amount * factor)) / factor; delete the earlier overwritten assignments so only the rounded approach remains. | if (direction === 'decrement') { | ||
| amount *= -1; | ||
| amount = -amount; | ||
| console.log(amount); | ||
| } |
There was a problem hiding this comment.
Remove debug console.log before merging.
Line 108 contains a debug console.log(amount) statement that should be removed.
Apply this diff:
if (direction === 'decrement') { amount = -amount; - console.log(amount); }📝 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.
| if (direction === 'decrement') { | |
| amount *= -1; | |
| amount = -amount; | |
| console.log(amount); | |
| } | |
| if (direction === 'decrement') { | |
| amount = -amount; | |
| } |
🤖 Prompt for AI Agents
In src/components/ui/NumberField/fragments/NumberFieldRoot.tsx around lines 106 to 109, remove the debug console.log(amount) call: delete the console.log(...) line so no debug logging remains in the decrement branch and ensure the surrounding logic remains unchanged. 1 participant
closes #1599
Summary by CodeRabbit
New Features
Documentation