Repository navigation
fix(NumberField): snap off-grid value to the nearest grid line when stepping - #2759
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesNumberField Step-Snapping
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: private package registry requires authentication. Disable ESLint in CodeRabbit settings or use public packages. 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 |
commit: |
![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
🤖 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 134-154: Update the disabled-state logic to use the same
step-aligned next value as the input handler in NumberFieldRoot.vue. The current
checks in isIncreaseDisabled and isDecreaseDisabled still rely on raw value
plus/minus step, so they can disagree with the new snapping behavior. Extract
the next-value calculation from the existing step handling path and reuse it in
both disabled helpers, then add a regression test covering the boundary cases
where alignment allows movement even though raw arithmetic would cross max or a
truthy min.
🪄 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: f9a68ae5-e53b-4c56-ba5b-9d241645838f
📒 Files selected for processing (2)
packages/core/src/NumberField/NumberField.test.tspackages/core/src/NumberField/NumberFieldRoot.vue
|
Thanks for the PR! The off-grid stepping fix (bug #2) looks correct — increment/decrement now snap to the nearest grid line in the right direction instead of overshooting. 👍 On bug #1 though, I don't think snapping typed values on blur is actually a bug. The current design — validating and snapping the committed value to the step grid — is the intended behavior. With If the goal is to allow off-grid typed values, that'd be better as an opt-in flag rather than changing the default commit behavior. The stepping fix can still land independently. |
isIncreaseDisabled/isDecreaseDisabled used the raw value ± step instead of the aligned next value, so a valid off-grid move (e.g. 8 -> 9 with max=10, step=3) was wrongly disabled. Both now reuse getNextValue, the same calc as the handler.
df521db to
0f5b088
Compare
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 149-151: The NaN fallback in NumberFieldRoot’s input handling
stores a raw 0/min fallback without enforcing the same bounds as normal input
updates. Update the branch in NumberFieldRoot’s value parsing logic so the
fallback value is passed through clampInputValue() before assigning to
modelValue, keeping the min/max contract consistent when the input is empty or
unparseable. Use the existing clampInputValue helper and the current modelValue
update path to locate the fix.
🪄 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: ae0f5d13-87ee-413f-963e-03b89815518d
📒 Files selected for processing (2)
packages/core/src/NumberField/NumberField.test.tspackages/core/src/NumberField/NumberFieldRoot.vue
xerox0213
changed the title
When the input is empty or unparseable, the fallback wrote `min ?? 0` directly, which violates the range when only a negative max is set. It now goes through clampInputValue so the same min/max contract applies.
@zernonia Thanks for the review! Agreed on bug #1 — you're right that snapping the committed value to the step grid is the intended behavior when stepSnapping is on. I've dropped that change, so the default commit behavior is unchanged (typing 2.5 with step=1 still snaps on blur). This PR now contains only the off-grid stepping fix, plus the related disabled-state consistency fix (isIncreaseDisabled/isDecreaseDisabled now reuse the same aligned next-value calc, so a valid off-grid move like 8 → 9 with max=10/step=3 is no longer wrongly disabled). I also routed the empty/NaN fallback through clampInputValue so a negative max isn't violated. For the off-grid use case, the opt-in lives in a separate PR: #2761 — an allowInvalid prop that keeps typed input exactly as entered (skips both range clamping and step snapping; stepping still clamps/snaps). I kept it as a single prop rather than splitting into range/step flags — happy to rename or split if you'd prefer. |
🔗 Linked issue
#2758
❓ Type of change
📚 Description
Fixes increment/decrement overshooting when the value is off the step grid.
Previously a tick added a full step and then rounded to nearest, so an off-grid value skipped a line (
18.98+ step1→20instead of19;18.11− step1→17instead of18). Stepping now snaps to the nearest grid line in the requested direction (HTMLstepUp/stepDownsemantics); values already on the grid still move by a full step.The +/- disabled state had the same root cause:
isIncreaseDisabled/isDecreaseDisabledused the rawvalue ± stepinstead of the aligned next value, so a valid off-grid move (e.g.8 → 9withmax=10,step=3) was wrongly disabled. Both now reuse the samegetNextValuecalculation as the handler.📝 Checklist
Summary by CodeRabbit
New Features
Bug Fixes