fix: update folder height when content changes dynamically#604
fix: update folder height when content changes dynamically#604dinakars777 wants to merge 5 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
🦋 Changeset detectedLatest commit: f32b583 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.
|
📝 WalkthroughWalkthroughThis PR refines ValueInput change handling (deferring immediate updates for number inputs) and overhauls folder toggle height logic to measure content, use ResizeObserver, and synchronize wrapper height on dynamic content changes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 f32b583:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/leva/src/hooks/useToggle.ts (1)
120-134: Consider handling ResizeObserver-triggered height updates after initial animation.After
fixHeightclears the inline height/overflow styles (line 121-122), subsequent ResizeObserver callbacks will re-apply an inline height viaupdateHeight(). This height style remains indefinitely since thetransitionendlistener was removed after the initial animation ({ once: true }).This isn't necessarily a bug—the wrapper will keep updating its height as content changes. However, you may want to either:
- Accept that post-expansion changes leave inline height (current behavior)
- Re-register the
fixHeightlistener when ResizeObserver detects changes (for consistent cleanup)If the current behavior is intentional, a brief comment would clarify this design choice.
🤖 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 120 - 134, The current logic clears inline height/overflow in fixHeight after the initial transition but leaves updateHeight-driven inline heights from the ResizeObserver in place; either document this intentional behavior or change the flow: inside the ResizeObserver callback (where updateHeight is called) re-register the transitionend listener to call fixHeight (or call a shared cleanup function) so that subsequent size changes also clear inline styles, referencing the fixHeight function, updateHeight call, the ResizeObserver instance, contentEl and ref (and the 'transitionend' event) to locate and update the code accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/leva/src/components/ValueInput/ValueInput.tsx`:
- Around line 87-92: The change causes ValueInput's onChange handler (in
ValueInput -> update) to call onUpdate for any non-number type, leaking realtime
commits beyond strings; make realtime commits opt-in by adding a liveUpdate
boolean prop to ValueInput and only call onUpdate inside update when liveUpdate
is true (keep existing number behavior). Update usages: enable liveUpdate from
packages/leva/src/plugins/String/String.tsx (pass liveUpdate={true}) and leave
other call sites (e.g., plugins/Color/Color.tsx) unchanged so they retain the
original commit-on-blur/Enter behavior.
In `@packages/leva/src/hooks/useToggle.ts`:
- Around line 113-118: The early return in useToggle (the block that checks
firstRender.current, sets it false, calls updateHeight(), then returns) prevents
the ResizeObserver from being initialized for components that mount
already-expanded; remove that return (or move the ResizeObserver initialization
earlier) so the observer setup always runs: keep the firstRender.current = false
and the initial updateHeight() call to avoid the first animation, but ensure the
ResizeObserver creation/observe logic and its cleanup (the code that currently
runs after that return) executes regardless of firstRender so dynamic content
will trigger updateHeight; adjust the cleanup to still disconnect the observer
in the effect cleanup.
---
Nitpick comments:
In `@packages/leva/src/hooks/useToggle.ts`:
- Around line 120-134: The current logic clears inline height/overflow in
fixHeight after the initial transition but leaves updateHeight-driven inline
heights from the ResizeObserver in place; either document this intentional
behavior or change the flow: inside the ResizeObserver callback (where
updateHeight is called) re-register the transitionend listener to call fixHeight
(or call a shared cleanup function) so that subsequent size changes also clear
inline styles, referencing the fixHeight function, updateHeight call, the
ResizeObserver instance, contentEl and ref (and the 'transitionend' event) to
locate and update the code accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 95c7a902-d961-408d-8a69-b5d87df2577b
📒 Files selected for processing (3)
.changeset/plenty-spies-provide.mdpackages/leva/src/components/ValueInput/ValueInput.tsxpackages/leva/src/hooks/useToggle.ts
- Guard against transitionend not firing when height already matches target - Update changeset to reflect folder height dynamic update
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/leva/src/hooks/useToggle.ts (1)
138-145: ResizeObserver implementation looks correct.This achieves the PR objective—dynamically added content will trigger
updateHeight()and recalculate the wrapper height. Cleanup properly disconnects the observer.Consider: If rapid content changes cause performance concerns, the callback could be wrapped in
requestAnimationFrameto batch updates. This is likely unnecessary for typical folder usage but worth noting.,
🤖 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 138 - 145, Wrap the ResizeObserver callback to batch updates using requestAnimationFrame: in the ResizeObserver callback that currently calls updateHeight(), schedule updateHeight via requestAnimationFrame and store the returned raf id (e.g., rafId variable); on teardown (the returned cleanup) cancel any pending frame with cancelAnimationFrame(rafId) in addition to disconnecting resizeObserver and removing the 'transitionend' listener (fixHeight) so rapid changes don't queue redundant updates. Ensure you reference the existing symbols resizeObserver, updateHeight, and fixHeight when adding the rafId handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/leva/src/hooks/useToggle.ts`:
- Around line 138-145: Wrap the ResizeObserver callback to batch updates using
requestAnimationFrame: in the ResizeObserver callback that currently calls
updateHeight(), schedule updateHeight via requestAnimationFrame and store the
returned raf id (e.g., rafId variable); on teardown (the returned cleanup)
cancel any pending frame with cancelAnimationFrame(rafId) in addition to
disconnecting resizeObserver and removing the 'transitionend' listener
(fixHeight) so rapid changes don't queue redundant updates. Ensure you reference
the existing symbols resizeObserver, updateHeight, and fixHeight when adding the
rafId handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 067d8f86-eb57-4918-9310-2148e396e7ba
📒 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
|
Thanks for the review! Addressed by using |
1 participant
Summary
Fixes
Fixes #601
Changes
useTogglehook to include a ResizeObserver that updates the wrapper height when content dimensions changeupdateHeight()helper function that recalculates and applies the content heightTesting
Summary by CodeRabbit