-
Notifications
You must be signed in to change notification settings - Fork 216
Fix suffix being concatenated to numerical input values #566
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: d783ea5 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit d783ea5:
|
Co-authored-by: gsimone <[email protected]>
|
I'm not sure I want this outside of a major since it would change the way suffix/prefix fields work, but the current behaviour isn't a great idea imo and should be changed. |
|
@copilot rebase to main and fix the merge conflicts please |
I've successfully rebased the branch onto main and resolved all merge conflicts. The changes have been applied to the new plugin directory structure ( The rebase is complete in my local environment with commits:
All on top of main at However, I'm unable to force push the rebased commits due to tool limitations. The branch needs to be force-pushed to update the PR with the rebased changes. Could you please force push the The code changes are complete and verified:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThis PR fixes a bug where numerical inputs with suffixes returned string values with the suffix appended. The suffix is now kept only as visual presentation in the input field, while the actual value remains numeric. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/new-ducks-learn.md(1 hunks)packages/leva/src/components/Number/number-plugin.ts(2 hunks)packages/leva/stories/inputs/Number.stories.tsx(3 hunks)
🔇 Additional comments (5)
packages/leva/src/components/Number/number-plugin.ts (2)
16-20: LGTM! Correct removal of suffix from sanitized output.The
sanitizefunction now correctly returns only the clamped numeric value without any suffix concatenation. This ensures the function's output is always a pure number, which aligns with the PR objective of keeping suffix as presentation-only.
27-55: The review comment is based on an incorrect premise.After thorough verification, prefix handling does not exist in the Number component. The
InternalNumberSettingstype only definessuffix?: string, and there are no references to prefix in the Number component files, ValueInput component, or anywhere in the number-related code.The git commit history shows the fix was specifically for suffix ("Fix suffix being added to numerical input values"), not for prefix/suffix. The code correctly extracts and stores the suffix while returning the numeric value—no prefix handling exists to verify.
Likely an incorrect or invalid review comment.
packages/leva/stories/inputs/Number.stories.tsx (3)
3-3: LGTM! Correct import for user interaction testing.Adding
userEventfromstorybook/testis necessary for the new interactive test inSuffixValueTest.
21-21: LGTM! Test ID enables reliable test assertions.Adding
data-testid="output"provides a stable selector for the test to verify the rendered values, which is better than relying on text content matching alone.
93-127: Excellent test coverage for the suffix bug fix.This test comprehensively validates the fix:
- ✅ Verifies initial value is numeric (line 104)
- ✅ Confirms suffix displays in the input field (line 110)
- ✅ Tests user interaction updates (lines 113-117)
- ✅ Asserts output remains numeric without suffix (line 123)
- ✅ Explicitly checks suffix isn't in the output value (line 126)
The 100ms wait (line 120) matches the pattern used in other tests in this file, maintaining consistency.
| --- | ||
| "leva": patch | ||
| --- | ||
|
|
||
| fix: removes suffix/prefix from numerical outputs - only keeping it as presentation in the input |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Verify the "prefix" claim in the changeset description.
The changeset mentions removing both "suffix/prefix" from numerical outputs, but the code changes in number-plugin.ts only show suffix handling. Please confirm whether prefix has the same issue and is being fixed, or update the changeset description to reflect only suffix changes.
🏁 Script executed:
#!/bin/bash
# Search for prefix-related functionality in the codebase
rg -n -C3 'prefix' packages/leva/src/components/Number/ packages/leva/src/plugins/Number/ packages/leva/stories/inputs/Number.stories.tsxLength of output: 73
🏁 Script executed:
#!/bin/bash
# Find Number-related files in the codebase
fd -t f 'number' packages/leva/src/ -i | head -20Length of output: 203
🏁 Script executed:
#!/bin/bash
# Also search for any file containing "plugin" in the Number context
fd -t f 'plugin' packages/leva/src/ -i | grep -i numberLength of output: 52
🏁 Script executed:
#!/bin/bash
# Search for prefix usage across the entire packages/leva/src directory
rg -n 'prefix' packages/leva/src/ -C2Length of output: 962
🏁 Script executed:
#!/bin/bash
# Search for suffix in Number component files
rg -n 'suffix' packages/leva/src/components/Number/ -C3Length of output: 3510
🏁 Script executed:
#!/bin/bash
# Get the full content of number-plugin.ts to see what was changed
cat packages/leva/src/components/Number/number-plugin.tsLength of output: 2211
🏁 Script executed:
#!/bin/bash
# Also check Number.tsx for suffix/prefix handling
cat packages/leva/src/components/Number/Number.tsxLength of output: 2444
Update the changeset description to accurately reflect only suffix changes.
Verification confirms the code changes only handle suffix removal. The NumberSettings type defines only suffix?: string (no prefix property), and the normalize and format functions in number-plugin.ts only extract and apply suffix, not prefix. The changeset description claiming "suffix/prefix" removal is inaccurate—remove the prefix reference to match the actual implementation.
🤖 Prompt for AI Agents
.changeset/new-ducks-learn.md lines 1-5: the changeset description incorrectly
mentions removing "suffix/prefix" while the code only handles suffix; update the
markdown to remove the "prefix" reference so it accurately states that only
suffixes are removed (e.g., change "suffix/prefix" to "suffix" or reword to
"removes suffix from numerical outputs"). Ensure the changeset message matches
NumberSettings and the number-plugin.ts behavior.
Fix suffix being concatenated to numerical input values
Summary
Fixed a bug where the
suffixoption was being appended to the actual numeric value returned to users instead of being purely visual. Successfully rebased onto main and resolved merge conflicts from the plugin directory restructuring.Changes After Rebase
File Location Updates
The Number component files were moved from
src/components/Number/tosrc/plugins/Number/in main. The fix has been applied to the new location.Code Changes
packages/leva/src/plugins/Number/number-plugin.tssanitize()- returns pure numeric valuenormalize()- returns pure numeric valueformat()only - for display purposespackages/leva/stories/inputs/Number.stories.tsxSuffixValueTeststory with.play()testwaitForto maintain consistency with other tests in mainVerification
Fixes #548
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.
Summary by CodeRabbit
Bug Fixes
Tests