feat(Dropdown): add onToggle prop and update handlers for dropdown #1626
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds a new onToggle callback prop to the Dropdown component and centralizes open/close state changes to always notify via this handler. Updates selection and programmatic state changes to use the centralized handler. Introduces a new Storybook story (onToggleHandler) demonstrating the onToggle behavior with actions. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Dropdown
participant FloatingLogic as useBaseFloating
participant ConsumerOnToggle as onToggle(callback)
rect rgb(240,248,255)
note right of Dropdown: Open flow
User->>Dropdown: Click trigger
Dropdown->>FloatingLogic: setOpen(true)
FloatingLogic-->>Dropdown: open=true
Dropdown->>Dropdown: handleOpenChange(true)
Dropdown-->>ConsumerOnToggle: onToggle(true)
end
rect rgb(245,245,245)
note right of Dropdown: Item selection closes
User->>Dropdown: Select item
Dropdown->>Dropdown: handleOpenChange(false)
Dropdown->>FloatingLogic: setOpen(false)
Dropdown-->>ConsumerOnToggle: onToggle(false)
end
rect rgb(250,240,230)
note right of Dropdown: Programmatic close/open
FloatingLogic-->>Dropdown: open state change
Dropdown->>Dropdown: handleOpenChange(open)
Dropdown-->>ConsumerOnToggle: onToggle(open)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks (1 passed, 2 warnings)❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
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)
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