fix: suffix should only be visual, not part of the value#605
Open
dinakars777 wants to merge 6 commits intopmndrs:mainfrom
Open
fix: suffix should only be visual, not part of the value#605dinakars777 wants to merge 6 commits intopmndrs:mainfrom
dinakars777 wants to merge 6 commits intopmndrs:mainfrom
Conversation
Previously, string inputs only committed changes to the store on blur. This change makes onUpdate fire on every keystroke for string inputs only - number inputs retain their original commit behavior (on blur/enter). Added comment explaining the type !== 'number' guard. Fixes pmndrs#599
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 7eb8b43 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 |
📝 WalkthroughWalkthroughFixes numeric-suffix handling, makes non-number inputs call Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
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 7eb8b43:
|
![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
🧹 Nitpick comments (1)
packages/leva/src/components/ValueInput/ValueInput.tsx (1)
87-92: RealtimeonUpdateshould be opt-in, not applied to all non-number inputs.At Line 91, every non-number keystroke is now committed. For inputs like color editors (
packages/leva/src/plugins/Color/Color.tsx), partial values can fail sanitize and get reverted viauseInputSetters, causing edit disruption.Suggested direction
export type ValueInputProps = { ... + liveUpdate?: boolean } export function ValueInput({ ... + liveUpdate = false, }: ValueInputProps) { ... onChange={update((value) => { onChange(value) - if (type !== "number") onUpdate(value) + if (liveUpdate && type !== 'number') onUpdate(value) })}Then enable
liveUpdateonly for plain string inputs that are safe to commit per keystroke.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/leva/src/components/ValueInput/ValueInput.tsx` around lines 87 - 92, The current ValueInput component calls onUpdate on every keystroke for all non-number inputs, which breaks plugins like Color that rely on sanitized partial values; change the condition inside the update callback so onUpdate is invoked only when the input is a plain string and liveUpdate is explicitly enabled (e.g., replace the `if (type !== "number") onUpdate(value)` logic with a check like `type === "string" && props.liveUpdate`), leaving number inputs and other specialized types (Color plugin / useInputSetters consumers) using their existing commit-on-blur/Enter behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/plenty-spies-provide.md:
- Line 5: Replace the incorrect changeset headline "fix: update string input
value in real-time" with a succinct note that describes the actual fix (the
number suffix persistence bug) and update the body to mention that numeric
suffixes are now persisted across saves/roundtrips; specifically edit the
changeset title line and message in .changeset/plenty-spies-provide.md so it
references the suffix storage/persistence fix and briefly describes the
user-facing impact (e.g., "fix: persist numeric suffix on inputs" and a one-line
summary about suffix persistence being fixed).
In `@packages/leva/src/hooks/useToggle.ts`:
- Around line 113-117: The early return in useToggle (the firstRender.current
branch) prevents the ResizeObserver setup and overflow reset from running, so
initially-open folders never observe descendant size changes and can remain
overflow-clipped; modify the firstRender handling in useToggle.ts so that after
setting firstRender.current = false and calling updateHeight() you do NOT return
early—ensure the subsequent ResizeObserver setup (the observe call that uses
contentRef) and the overflow reset logic still run on first open (or,
alternatively, explicitly call the same observer setup and clear the wrapper
overflow before returning), referencing firstRender, updateHeight, contentRef
and the ResizeObserver/observe call.
- Around line 107-110: When collapsing (when !toggled) measure the element's
current pixel height and apply it as an inline height before transitioning to
'0px' so the CSS height transition can animate; inside useToggle (the block that
currently sets ref.style.height='0px'), read ref.offsetHeight (or
getBoundingClientRect().height), set ref.style.height = `${height}px`, then on
the next frame (requestAnimationFrame or setTimeout 0) set ref.style.height =
'0px' and only after the transition finishes set ref.style.overflow='hidden' and
call fixHeight() as needed—this ensures the element transitions from a concrete
pixel height instead of from height:auto.
In `@packages/leva/src/plugins/Number/number-plugin.ts`:
- Around line 53-54: sanitize() currently appends the suffix (return suffix ? f
+ suffix : f) which causes updates to persist suffixed strings; change sanitize
so it returns a numeric value suitable for storage (no suffix) and move the
suffix-appending logic into a separate formatting step (or add an optional
boolean param like sanitize(value, { forStorage: true })) so normalize() and the
update path store numbers, while a new format/display function handles adding
suffixes for UI only; update calls to sanitize/normalize in number-plugin.ts to
use the storage-mode sanitize or the new formatter where appropriate.
---
Nitpick comments:
In `@packages/leva/src/components/ValueInput/ValueInput.tsx`:
- Around line 87-92: The current ValueInput component calls onUpdate on every
keystroke for all non-number inputs, which breaks plugins like Color that rely
on sanitized partial values; change the condition inside the update callback so
onUpdate is invoked only when the input is a plain string and liveUpdate is
explicitly enabled (e.g., replace the `if (type !== "number") onUpdate(value)`
logic with a check like `type === "string" && props.liveUpdate`), leaving number
inputs and other specialized types (Color plugin / useInputSetters consumers)
using their existing commit-on-blur/Enter behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: addee39e-af6b-42e0-ac70-4cc0a0cd3883
📒 Files selected for processing (4)
.changeset/plenty-spies-provide.mdpackages/leva/src/components/ValueInput/ValueInput.tsxpackages/leva/src/hooks/useToggle.tspackages/leva/src/plugins/Number/number-plugin.ts
- Guard against transitionend not firing when height already matches target - Update changeset to reflect actual fixes (suffix persistence, folder height)
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/leva/src/hooks/useToggle.ts (2)
113-118:⚠️ Potential issue | 🟠 MajorInitially-open folders never observe nested content changes.
The early return at line 117 prevents the
ResizeObserversetup (lines 138-141) from running. PerFolder.tsx(lines 60-66), nestedFoldercomponents render insidecontentRef. When a parent folder starts expanded, toggling nested folders won't update the parent wrapper's height.Move ResizeObserver setup before the early return or restructure to ensure observation starts on first render:
💡 Proposed fix
+ const resizeObserver = new ResizeObserver(() => { + updateHeight() + }) + resizeObserver.observe(contentEl) + // prevents first animation on initial expand if (firstRender.current) { firstRender.current = false updateHeight() - return + return () => { + resizeObserver.disconnect() + } } const fixHeight = () => { ref.style.removeProperty('height') ref.style.removeProperty('overflow') contentEl.scrollIntoView({ behavior: 'smooth', block: 'nearest' }) } ref.addEventListener('transitionend', fixHeight, { once: true }) const { height } = contentEl.getBoundingClientRect() const currentHeight = ref.style.height ref.style.height = `${height}px` // If no transition will occur, call fixHeight directly if (currentHeight === `${height}px`) { ref.removeEventListener('transitionend', fixHeight) fixHeight() } - const resizeObserver = new ResizeObserver(() => { - updateHeight() - }) - resizeObserver.observe(contentEl) - return () => { ref.removeEventListener('transitionend', fixHeight) resizeObserver.disconnect() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/leva/src/hooks/useToggle.ts` around lines 113 - 118, The early return in the useToggle hook using firstRender.current prevents the ResizeObserver from being attached, so initially-open Folder components never observe nested content changes; fix by ensuring the ResizeObserver for contentRef is initialized before returning on first render (or by moving the ResizeObserver setup out of the post-firstRender branch) so updateHeight() still runs but observation starts immediately—modify the logic around firstRender.current, updateHeight(), and the ResizeObserver setup in useToggle so the observer is registered on first render even when returning early.
107-111:⚠️ Potential issue | 🟠 MajorCollapse animation breaks after first expand.
After
fixHeightclears the inline height (line 121), the wrapper returns toheight: auto. On subsequent collapse, settingheight: 0pxdirectly fromautowon't animate—CSS transitions require a concrete start value.Measure the current height before collapsing to enable the transition:
💡 Proposed fix
if (!toggled) { + const { height } = contentEl.getBoundingClientRect() + ref.style.height = `${height}px` ref.style.overflow = 'hidden' - ref.style.height = '0px' + requestAnimationFrame(() => { + ref.style.height = '0px' + }) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/leva/src/hooks/useToggle.ts` around lines 107 - 111, The collapse animation breaks because after fixHeight clears the inline height (making the wrapper height auto) later collapses set height directly to '0px' which cannot transition from 'auto'; in useToggle, when toggled becomes false, first read the element's current computed height (e.g., via getBoundingClientRect().height), set ref.style.height to that measured px value, force a reflow (read offsetHeight) and then set ref.style.height = '0px' so the CSS transition animates; ensure fixHeight still clears the inline height after the transition ends (or on transitionend) to restore auto.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/leva/src/hooks/useToggle.ts`:
- Around line 113-118: The early return in the useToggle hook using
firstRender.current prevents the ResizeObserver from being attached, so
initially-open Folder components never observe nested content changes; fix by
ensuring the ResizeObserver for contentRef is initialized before returning on
first render (or by moving the ResizeObserver setup out of the post-firstRender
branch) so updateHeight() still runs but observation starts immediately—modify
the logic around firstRender.current, updateHeight(), and the ResizeObserver
setup in useToggle so the observer is registered on first render even when
returning early.
- Around line 107-111: The collapse animation breaks because after fixHeight
clears the inline height (making the wrapper height auto) later collapses set
height directly to '0px' which cannot transition from 'auto'; in useToggle, when
toggled becomes false, first read the element's current computed height (e.g.,
via getBoundingClientRect().height), set ref.style.height to that measured px
value, force a reflow (read offsetHeight) and then set ref.style.height = '0px'
so the CSS transition animates; ensure fixHeight still clears the inline height
after the transition ends (or on transitionend) to restore auto.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bc73a8c5-8c8b-47be-b47b-64255eb5944e
📒 Files selected for processing (2)
.changeset/plenty-spies-provide.mdpackages/leva/src/hooks/useToggle.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/plenty-spies-provide.md
Author
|
Thanks for the review! Updated changeset to reflect actual fixes: number suffix persistence and folder height dynamic updates. Resolved. |
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.
Summary
formatfunction, while the stored value remains a pure numberFixes
Fixes #548
Changes
normalizefunction innumber-plugin.tsto return just the numeric value (_value) instead of appending the suffixTesting
Summary by CodeRabbit