feat(Dropdown): add onToggle prop and update handlers for dropdown#1626
feat(Dropdown): add onToggle prop and update handlers for dropdown#1626kaifcoder wants to merge 3 commits intothemesberg:mainfrom
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds an Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Dropdown
participant Floating as useBaseFloating
participant Consumer as onToggle(callback)
rect rgb(240,248,255)
note right of Dropdown: User opens dropdown
User->>Dropdown: Click trigger
Dropdown->>Floating: setOpen(true)
Floating-->>Dropdown: open=true
Dropdown->>Dropdown: handleOpenChange(true)
Dropdown-->>Consumer: onToggle(true)
end
rect rgb(245,245,245)
note right of Dropdown: User selects an item (closes)
User->>Dropdown: Select item
Dropdown->>Dropdown: handleOpenChange(false)
Dropdown->>Floating: setOpen(false)
Dropdown-->>Consumer: onToggle(false)
end
rect rgb(250,240,230)
note right of Dropdown: Programmatic open/close
Floating-->>Dropdown: open state change
Dropdown->>Dropdown: handleOpenChange(open)
Dropdown-->>Consumer: onToggle(open)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)packages/ui/src/components/Dropdown/Dropdown.tsx (2)
🔇 Additional comments (4)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
packages/ui/src/components/Dropdown/Dropdown.tsx (6)
48-49: Don’t Omit a non-existent key from ButtonProps
onToggleisn’t a member ofButtonProps, so including it in theOmitadds noise. Keeping theOmitminimal improves clarity.- extends Pick<FloatingProps, "placement" | "trigger">, - Omit<ButtonProps, keyof ThemingProps<DropdownTheme> | "onToggle">, + extends Pick<FloatingProps, "placement" | "trigger">, + Omit<ButtonProps, keyof ThemingProps<DropdownTheme>>,
57-57: Add concise TSDoc to define onToggle semanticsDocument when
onTogglefires (only on state changes vs. every call). This avoids consumer confusion.- onToggle?: (open: boolean) => void; + /** + * Called when the open state changes due to user interaction or programmatic updates. + * Receives the next `open` value. + */ + onToggle?: (open: boolean) => void;
148-155: Fire onToggle only when state actually changesCurrently
onToggleis invoked even ifnewOpenequals the current state, causing redundant callbacks (extra Storybook actions, potential double logs in StrictMode). Guard and includeopenin deps.- const handleOpenChange = useCallback( - (newOpen: boolean) => { - setOpen(newOpen); - onToggle?.(newOpen); - }, - [onToggle], - ); + const handleOpenChange = useCallback( + (newOpen: boolean) => { + if (newOpen === open) return; + setOpen(newOpen); + onToggle?.(newOpen); + }, + [open, onToggle], + );
156-162: Avoid closing (and notifying) when already closedWhen typeahead selects while closed,
handleSelectcallshandleOpenChange(false), which triggersonToggle(false)unnecessarily. Close only if open.- const handleSelect = useCallback( - (index: number | null) => { - setSelectedIndex(index); - handleOpenChange(false); - }, - [handleOpenChange], - ); + const handleSelect = useCallback( + (index: number | null) => { + setSelectedIndex(index); + if (open) handleOpenChange(false); + }, + [open, handleOpenChange], + );
177-181: Minor: stabilize the setOpen bridge to avoid new function per renderNot critical, but passing a stable callback reduces churn if
useBaseFLoatingtracks handler identity.- const { context, floatingStyles, refs } = useBaseFLoating<HTMLButtonElement>({ - open, - setOpen: (value) => { - const newOpen = typeof value === "function" ? value(open) : value; - handleOpenChange(newOpen); - }, - placement, - }); + const setOpenWithNotify = useCallback( + (value: SetStateAction<boolean>) => { + const next = typeof value === "function" ? (value as (prev: boolean) => boolean)(open) : value; + handleOpenChange(next); + }, + [open, handleOpenChange], + ); + + const { context, floatingStyles, refs } = useBaseFLoating<HTMLButtonElement>({ + open, + setOpen: setOpenWithNotify, + placement, + });
46-59: API addition: ensure docs and migration notes are updatedThis is a new public prop. Please add it to the component docs (Props table), mention behavior in “Events”, and note in the PR description whether it’s a breaking change (it isn’t) and any migration hints.
I can open a docs PR. If helpful, I’ll also add a brief “onToggle” example mirroring the Storybook story.
apps/storybook/src/Dropdown.stories.tsx (1)
133-146: Nice addition; covers the new callbackStory cleanly demonstrates
onToggle. Consider capitalizing the story name for consistency with others (“OnToggle handlers”) and optionally adding a quick note in the story description about firing on open/close.-onToggleHandler.storyName = "onToggle handlers"; +onToggleHandler.storyName = "OnToggle handlers";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/storybook/src/Dropdown.stories.tsx(1 hunks)packages/ui/src/components/Dropdown/Dropdown.tsx(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/ui/src/components/Dropdown/Dropdown.tsx (2)
packages/ui/src/components/Button/Button.tsx (1)
ButtonProps(48-59)packages/ui/src/types/index.ts (1)
ThemingProps(20-46)
|
@SutuSebastian is attempting to deploy a commit to the Themesberg Team on Vercel. A member of the Team first needs to authorize it. |
2 participants
Summarize the changes made and the motivation behind them.
Addressed issue number
#1617Reference related issues using
#followed by the issue number.If there are breaking API changes - like adding or removing props, or changing the structure of the theme - describe them, and provide steps to update existing code.
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.