feat(Number Field): add startingValue prop for increment/decrement#2628
feat(Number Field): add startingValue prop for increment/decrement#2628rikzwarthoff wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughAdded an optional ChangesNumberField Starting Value
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 1
🧹 Nitpick comments (1)
packages/core/src/NumberField/NumberField.test.ts (1)
94-108: ⚡ Quick winAdd edge-case coverage for
startingValueoutside[min, max]bounds.The new test only covers the happy path (no
min/max). A test withstartingValueoutside the configured range would expose the missingclampInputValue()call flagged above, and document the expected clamping behavior going forward.✅ Suggested additional test
it('should clamp startingValue to min/max on first increment/decrement', async () => { // startingValue below min — first increment should clamp to min const { input, increment } = setup({ min: 10, max: 20, step: 3, startingValue: 5 }) expect(input.value).toBe('') await userEvent.click(increment) expect(input.value).toBe('10') // clamped to min, snapped to step })🤖 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/core/src/NumberField/NumberField.test.ts` around lines 94 - 108, Add a test to cover startingValue outside min/max so startingValue is clamped before the first increment/decrement: update NumberField.test.ts to add a case using setup({ min, max, step, startingValue }) (e.g., startingValue < min and startingValue > max) and assert that after the first userEvent.click(increment) or click(decrement) the input.value equals the clamped/snapped value; reference the same setup helper and ensure behavior matches clampInputValue() (i.e., snapped to step and bounded by min/max) for both increment and decrement flows.
🤖 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/core/src/NumberField/NumberFieldRoot.vue`:
- Line 124: The NaN-branch assigns startingValue directly to modelValue.value
without enforcing bounds or step alignment; replace that direct assignment by
passing startingValue through the existing clampInputValue() logic (the same
function used elsewhere) before setting modelValue.value so the initial value is
clamped/snapped to [min, max] and step rules; update the assignment at
modelValue.value = startingValue.value ?? min.value ?? 0 to call
clampInputValue(startingValue.value) (or equivalent) and preserve the fallback
to min.value ?? 0 if startingValue is undefined/NaN.
---
Nitpick comments:
In `@packages/core/src/NumberField/NumberField.test.ts`:
- Around line 94-108: Add a test to cover startingValue outside min/max so
startingValue is clamped before the first increment/decrement: update
NumberField.test.ts to add a case using setup({ min, max, step, startingValue })
(e.g., startingValue < min and startingValue > max) and assert that after the
first userEvent.click(increment) or click(decrement) the input.value equals the
clamped/snapped value; reference the same setup helper and ensure behavior
matches clampInputValue() (i.e., snapped to step and bounded by min/max) for
both increment and decrement flows.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: dd647a25-99a9-41ca-9f45-50285f34877d
📒 Files selected for processing (2)
packages/core/src/NumberField/NumberField.test.tspackages/core/src/NumberField/NumberFieldRoot.vue
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/core/src/NumberField/NumberFieldRoot.vue`:
- Around line 123-125: The code clamps the parsed NaN input by calling
clampInputValue(startingValue.value ?? min.value ?? 0), but snapValueToStep
currently treats an undefined lower bound as 0 which causes a startingValue
(e.g., 40 with step 3) to snap down to 39; update clamp behavior so that when
startingValue is explicitly provided and min is undefined you preserve the raw
startingValue (do not call snapValueToStep against an implied 0), i.e., change
the logic around clampInputValue / snapValueToStep invocation to only snap to
step when a concrete lower bound (min) exists or when startingValue is absent,
referencing clampInputValue and snapValueToStep in NumberFieldRoot.vue and the
startingValue/min variables.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4edcc05f-b072-4ffb-8d9c-de579a98b15d
📒 Files selected for processing (1)
packages/core/src/NumberField/NumberFieldRoot.vue
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/core/src/NumberField/NumberFieldRoot.vue (1)
123-125:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
clampInputValuesnapsstartingValueaway from the intended value whenminis undefined.
clampInputValuedelegates tosnapValueToStep(val, min, max, step). Whenminisundefined,snapValueToStepanchors the grid at0. With the test's own configuration (startingValue: 40,step: 3, nomin):(40 − 0) % 3 = 1 → snaps down to 39The first increment will write
39instead of the expected40, and subsequent assertions (43,40,37) are all offset by−1, so the entire test will fail.Semantically,
startingValuerepresents an exact starting point, not a point that must align with the step grid. Only min/max clamping should apply here.🛡️ Proposed fix
- modelValue.value = startingValue.value !== undefined ? clampInputValue(startingValue.value) : min.value ?? 0 + modelValue.value = startingValue.value !== undefined ? clamp(startingValue.value, min.value, max.value) : min.value ?? 0
clamp(already imported) enforces[min, max]bounds without snapping to the step grid, preserving the user's exactstartingValue. Subsequent increments/decrements from that value will still be step-aligned.🤖 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/core/src/NumberField/NumberFieldRoot.vue` around lines 123 - 125, The code uses clampInputValue (which snaps via snapValueToStep) when initializing modelValue from startingValue causing unwanted step snapping; instead, preserve the exact startingValue and only enforce min/max bounds using the existing clamp helper. Replace the clampInputValue call in the isNaN branch with a clamp of startingValue.value against min and max (use min.value and max.value or their undefined-aware defaults) so startingValue is not snapped to the step grid while still respecting bounds; keep all other logic (modelValue, startingValue, snapValueToStep) unchanged.
🤖 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.
Duplicate comments:
In `@packages/core/src/NumberField/NumberFieldRoot.vue`:
- Around line 123-125: The code uses clampInputValue (which snaps via
snapValueToStep) when initializing modelValue from startingValue causing
unwanted step snapping; instead, preserve the exact startingValue and only
enforce min/max bounds using the existing clamp helper. Replace the
clampInputValue call in the isNaN branch with a clamp of startingValue.value
against min and max (use min.value and max.value or their undefined-aware
defaults) so startingValue is not snapped to the step grid while still
respecting bounds; keep all other logic (modelValue, startingValue,
snapValueToStep) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8175c21f-13b2-4b92-94e3-05ec7453ea66
📒 Files selected for processing (1)
packages/core/src/NumberField/NumberFieldRoot.vue
2 participants
🔗 Linked issue
(#2627)
❓ Type of change
📚 Description
Added new prop 'startingValue' to Number Field
When no value is present and startingValue is given, increment/decrement will fill the startingValue. If startingValue is not present it will fallback to previous behavior, min value or zero.
See the issue for an useful example.
📝 Checklist
Summary by CodeRabbit
New Features
Tests