fix: button label should use settings.label if provided#606
fix: button label should use settings.label if provided#606dinakars777 wants to merge 7 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: 1077577 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 |
📝 WalkthroughWalkthroughButton now uses 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 1077577:
|
![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 (2)
packages/leva/src/components/Button/Button.tsx (1)
11-20: LGTM! Implementation correctly fixes issue#528.The logic properly prioritizes
settings.labelwhen provided, falling back to the key-basedlabelprop otherwise.Minor optional simplification: since
settings.labelis typed asstring | undefined, you could use the nullish coalescing operator for slightly cleaner code:♻️ Optional: Use nullish coalescing
export function Button({ onClick, settings, label }: ButtonProps) { const store = useStoreContext() - const displayLabel = settings.label !== undefined ? settings.label : label + const displayLabel = settings.label ?? label return (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/leva/src/components/Button/Button.tsx` around lines 11 - 20, In Button (function Button) simplify the label fallback by replacing the ternary that reads settings.label !== undefined ? settings.label : label with a nullish coalescing expression so the component uses settings.label when present and falls back to label otherwise; update the variable displayLabel in Button to use the nullish coalescing form to make the intent clearer and more concise.packages/leva/src/hooks/useToggle.ts (1)
100-105: Theheight > 0guard may cause stale wrapper height when content shrinks to zero.If contentEl shrinks from a positive height to 0 (e.g., all children removed while toggled is true), the ResizeObserver fires but
updateHeight()skips the update due toheight > 0. The wrapper retains its previous height.Consider whether content legitimately shrinking to 0 is a supported scenario; if so, handle it explicitly.
Potential approach if shrink-to-zero must be supported
const updateHeight = () => { const { height } = contentEl.getBoundingClientRect() - if (ref.style.height !== `${height}px` && height > 0) { + if (ref.style.height !== `${height}px`) { ref.style.height = `${height}px` } }However, this may introduce flicker if height is temporarily 0 during opacity transitions. You may need a debounce or explicit check for the collapsed state.
🤖 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 100 - 105, The updateHeight function currently ignores updates when contentEl.getBoundingClientRect().height is 0 (the height > 0 guard), which leaves the wrapper's ref.style.height stale when content shrinks to zero; change updateHeight (used by the ResizeObserver) to explicitly handle a 0 height case by allowing ref.style.height to be set to "0px" when the measured height is 0 or when the component is collapsed (check the toggled/expanded state if present), and if needed debounce or gate the 0px assignment during known opacity/transition windows to avoid flicker; locate updateHeight, contentEl, ref and the ResizeObserver callback in useToggle.ts to implement this logic.
🤖 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/hooks/useToggle.ts`:
- Around line 126-129: When setting ref.style.height after computing const {
height } = contentEl.getBoundingClientRect(), guard against the case where the
inline height already equals `${height}px` so that fixHeight still runs; add a
check comparing ref.style.height to the target string and if they match call
fixHeight() immediately (or schedule via requestAnimationFrame) instead of
relying solely on ref.addEventListener('transitionend', fixHeight, { once: true
}), and also add a short fallback timeout (e.g., setTimeout to call fixHeight
after ~300ms) to ensure fixHeight executes if the transition never fires; update
the code paths around ref.addEventListener, fixHeight, and the block that sets
ref.style.height/contentEl to incorporate these guards.
---
Nitpick comments:
In `@packages/leva/src/components/Button/Button.tsx`:
- Around line 11-20: In Button (function Button) simplify the label fallback by
replacing the ternary that reads settings.label !== undefined ? settings.label :
label with a nullish coalescing expression so the component uses settings.label
when present and falls back to label otherwise; update the variable displayLabel
in Button to use the nullish coalescing form to make the intent clearer and more
concise.
In `@packages/leva/src/hooks/useToggle.ts`:
- Around line 100-105: The updateHeight function currently ignores updates when
contentEl.getBoundingClientRect().height is 0 (the height > 0 guard), which
leaves the wrapper's ref.style.height stale when content shrinks to zero; change
updateHeight (used by the ResizeObserver) to explicitly handle a 0 height case
by allowing ref.style.height to be set to "0px" when the measured height is 0 or
when the component is collapsed (check the toggled/expanded state if present),
and if needed debounce or gate the 0px assignment during known
opacity/transition windows to avoid flicker; locate updateHeight, contentEl, ref
and the ResizeObserver callback in useToggle.ts to implement this logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ba6bb1b-d14f-4efd-97fa-d6a4f1c090cd
📒 Files selected for processing (6)
.changeset/plenty-spies-provide.mdpackages/leva/src/components/Button/Button.tsxpackages/leva/src/components/ValueInput/ValueInput.tsxpackages/leva/src/hooks/useToggle.tspackages/leva/src/plugins/Number/number-plugin.tspackages/leva/src/types/public.ts
- Guard against transitionend not firing when height already matches target - Update changeset to reflect button label fix
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/hooks/useToggle.ts`:
- Around line 114-118: The early return in useToggle (the firstRender.current
branch) prevents the ResizeObserver from being created when toggled starts true;
change the flow so you still call updateHeight() and then proceed to
create/register the ResizeObserver (or call the existing resize observer setup
code) even on first render, but keep the animation/skipping logic gated by
firstRender.current so the initial render doesn't animate; adjust the code
around firstRender.current, updateHeight, and the ResizeObserver setup (or
setupResizeObserver function) to ensure the observer is always attached while
preserving the no-animation-on-first-render behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b6e4b6f7-757e-491d-adb7-ccccc907ff51
📒 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! Added transitionend guard to handle edge case when height already matches target. Updated changeset. Resolved. |
1 participant
Summary
settings.labelwas being ignoredsettings.labelif provided, otherwise falls back to the key namelabel?: stringtoButtonSettingstypeFixes
Fixes #528
Changes
Buttoncomponent to prefersettings.labelover the key-based labelButtonSettingstype to include optionallabelpropertyTesting
Summary by CodeRabbit
New Features
Bug Fixes