feat(popover): update dropdown animation#840
Conversation
… usage in various sections
…essary modal props
… CollectionOptionsPopover
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a local Popover wrapper and AnimatedSize component, migrates several dropdowns/popovers to the new wrapper or Menu-based animated flows, and standardizes Popup/Positioner classNames and animation data-attributes across UI primitives and dashboard popovers. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/pageComponents/dashboard/dashboardLayout/header-options-popover.tsx (1)
169-171: Verify sideOffset change doesn't cause visual regression.The
Positionernow uses the wrapper's defaultsideOffset={1}instead of a potentially larger value. Ensure the sub-panel positioning still looks correct visually.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pageComponents/dashboard/dashboardLayout/header-options-popover.tsx` around lines 169 - 171, The Positioner change reduced sideOffset to the wrapper default (sideOffset={1}), which may shift the Popover.Popup; visually inspect Popover.Positioner and Popover.Popup placements for both view === "share" and other views and, if you see misalignment, restore or tweak sideOffset on Popover.Positioner (or adjust Popup alignment) to match the previous layout—update the sideOffset value and re-check the sub-panel positioning across breakpoints and anchor variations until visuals match the original.src/pageComponents/dashboard/sidePane/collection-options-popover.tsx (1)
161-164: Optional: Consider removing redundantbg-gray-50from inner div.The Popup wrapper already applies
bg-gray-50. The inner div'sbg-gray-50is redundant since they're the same color. However, this is pre-existing code outside the refactor scope.♻️ Optional cleanup
<Popover.Popup className="leading-[20px]"> - <div className="w-75 rounded-lg bg-gray-50"> + <div className="w-75 rounded-lg"> <ShareContent categoryId={categoryId} /> </div> </Popover.Popup>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pageComponents/dashboard/sidePane/collection-options-popover.tsx` around lines 161 - 164, Remove the redundant bg-gray-50 from the inner div inside the Popover.Popup so the Popup's background styling isn't duplicated; update the JSX where Popover.Popup contains the div (the element with className "w-75 rounded-lg bg-gray-50") to only include the necessary classes (e.g., "w-75 rounded-lg") and leave ShareContent(categoryId) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/pageComponents/dashboard/dashboardLayout/header-options-popover.tsx`:
- Around line 169-171: The Positioner change reduced sideOffset to the wrapper
default (sideOffset={1}), which may shift the Popover.Popup; visually inspect
Popover.Positioner and Popover.Popup placements for both view === "share" and
other views and, if you see misalignment, restore or tweak sideOffset on
Popover.Positioner (or adjust Popup alignment) to match the previous
layout—update the sideOffset value and re-check the sub-panel positioning across
breakpoints and anchor variations until visuals match the original.
In `@src/pageComponents/dashboard/sidePane/collection-options-popover.tsx`:
- Around line 161-164: Remove the redundant bg-gray-50 from the inner div inside
the Popover.Popup so the Popup's background styling isn't duplicated; update the
JSX where Popover.Popup contains the div (the element with className "w-75
rounded-lg bg-gray-50") to only include the necessary classes (e.g., "w-75
rounded-lg") and leave ShareContent(categoryId) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0e251e4b-be78-431b-a2be-b59735c5b1a1
📒 Files selected for processing (11)
src/components/clearTrashDropdown.tsxsrc/components/customDropdowns.tsx/addBookmarkDropdown.tsxsrc/components/customDropdowns.tsx/bookmarksViewDropdown.tsxsrc/components/customDropdowns.tsx/categoryIconsDropdown.tsxsrc/components/ui/recollect/combobox/index.tsxsrc/components/ui/recollect/menu.tsxsrc/components/ui/recollect/popover.tsxsrc/components/ui/recollect/select.tsxsrc/pageComponents/dashboard/cardSection/edit-popover.tsxsrc/pageComponents/dashboard/dashboardLayout/header-options-popover.tsxsrc/pageComponents/dashboard/sidePane/collection-options-popover.tsx
…nimations in popovers
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/pageComponents/dashboard/sidePane/collection-options-popover.tsx (1)
139-140: Usecn()for this conditional class.This hand-built template literal makes the visibility rule harder to scan and drifts from the repo's className convention. Switching this to
cn(...)would keep it consistent with the rest of the codebase.As per coding guidelines, "Use
cn()helper for conditional classes instead of string concatenation".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pageComponents/dashboard/sidePane/collection-options-popover.tsx` around lines 139 - 140, Replace the hand-built template literal on the <p> element's className with the cn(...) helper: ensure cn is imported, then call cn('block text-right text-[11px] leading-[115%] font-450 tracking-[0.03em] text-gray-600 group-hover:hidden', { hidden: showTrigger }) so the visibility toggling uses cn and preserves the existing classes and group-hover:hidden behavior; update the component where the <p> uses showTrigger to use this cn call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pageComponents/dashboard/sidePane/collection-options-popover.tsx`:
- Around line 68-119: The share panel currently renders inside Menu.Popup (see
Menu.Popup usage and the view === "share" branch that renders <ShareContent
categoryId={item.id} />), which forces menu semantics; replace that branch so
the "share" view uses the Popover primitive (the Popover component used
elsewhere in the app) instead of Menu.Popup: move the share panel out of
Menu.Popup or swap the wrapper only for the share branch to render a
Popover.Positioner/Popover.Content (or equivalent Popover API in your codebase)
that contains the same ShareContent markup, ensuring Menu.Item remains for the
"menu" view and keyboard/screen-reader semantics are correct.
---
Nitpick comments:
In `@src/pageComponents/dashboard/sidePane/collection-options-popover.tsx`:
- Around line 139-140: Replace the hand-built template literal on the <p>
element's className with the cn(...) helper: ensure cn is imported, then call
cn('block text-right text-[11px] leading-[115%] font-450 tracking-[0.03em]
text-gray-600 group-hover:hidden', { hidden: showTrigger }) so the visibility
toggling uses cn and preserves the existing classes and group-hover:hidden
behavior; update the component where the <p> uses showTrigger to use this cn
call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 309d46e7-54ce-4e65-bad2-4294069eb8a0
📒 Files selected for processing (2)
src/pageComponents/dashboard/dashboardLayout/header-options-popover.tsxsrc/pageComponents/dashboard/sidePane/collection-options-popover.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pageComponents/dashboard/dashboardLayout/header-options-popover.tsx
| <Menu.Positioner align="start"> | ||
| <Menu.Popup className="leading-[20px]"> | ||
| <FavoriteMenuItem | ||
| categoryId={item.id} | ||
| isFavorite={item.isFavorite} | ||
| /> | ||
| <Menu.Item | ||
| onClick={(event) => { | ||
| event.stopPropagation(); | ||
| setView("share"); | ||
| }} | ||
| > | ||
| Share | ||
| </Menu.Item> | ||
| <Menu.Item | ||
| onClick={(event) => { | ||
| event.stopPropagation(); | ||
| setView("delete"); | ||
| }} | ||
| > | ||
| Delete | ||
| </Menu.Item> | ||
| <Menu.Popup className="w-auto overflow-clip p-0 leading-[20px]"> | ||
| <AnimatedSize> | ||
| <AnimatePresence mode="popLayout" initial={false}> | ||
| {view === "menu" && ( | ||
| <motion.div | ||
| key="menu" | ||
| className="w-48 p-1" | ||
| initial={{ opacity: 0 }} | ||
| animate={{ opacity: 1 }} | ||
| exit={{ opacity: 0 }} | ||
| transition={fade} | ||
| > | ||
| <FavoriteMenuItem | ||
| categoryId={item.id} | ||
| isFavorite={item.isFavorite} | ||
| /> | ||
| <Menu.Item | ||
| onClick={(event) => { | ||
| event.stopPropagation(); | ||
| isItemClickRef.current = true; | ||
| setView("share"); | ||
| }} | ||
| > | ||
| Share | ||
| </Menu.Item> | ||
| <Menu.Item | ||
| onClick={(event) => { | ||
| event.stopPropagation(); | ||
| isItemClickRef.current = true; | ||
| setView("delete"); | ||
| }} | ||
| > | ||
| Delete | ||
| </Menu.Item> | ||
| </motion.div> | ||
| )} | ||
| {view === "share" && ( | ||
| <motion.div | ||
| key="share" | ||
| className="p-1" | ||
| initial={{ opacity: 0 }} | ||
| animate={{ opacity: 1 }} | ||
| exit={{ opacity: 0 }} | ||
| transition={fade} | ||
| > | ||
| <div className="w-75 rounded-lg bg-gray-50"> | ||
| <ShareContent categoryId={item.id} /> | ||
| </div> | ||
| </motion.div> | ||
| )} | ||
| </AnimatePresence> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Base UI (@base-ui/react), what ARIA role and keyboard behavior does Menu.Popupapply, and is it supported to render arbitrary interactive content like a share panel inside it instead ofMenu.Item descendants?
💡 Result:
Menu.Popup: ARIA role + keyboard model
- ARIA role:
Menu.Popupultimately receivesrole="menu"(viauseRole(..., { role: 'menu' })inMenuRoot). [1] - Keyboard behavior (composite “menu” pattern):
- Roving focus across registered menu items (items get
role="menuitem"andtabIndex={0|-1}depending on the highlighted/active item). [2] - Arrow-key navigation (via
useListNavigation, withorientationcontrolling whether it’s up/down vs left/right), including optional looping (loopFocus). [1] - Typeahead (first-character/text navigation) (via
useTypeahead). [1] - Dismiss behaviors including Escape/outside press (via
useDismiss). [1]
- Roving focus across registered menu items (items get
Can you render arbitrary interactive content (e.g. a share panel) inside Menu.Popup?
Not as a supported/accessible use of the Menu primitive. Base UI’s Menu is built as a list of actions with menuitem semantics and composite navigation (“roving focus” + typeahead) [3]; arbitrary interactive controls (buttons, inputs, etc.) inside role="menu" generally won’t participate correctly in that model (they won’t be in the menu’s item registry for roving focus/typeahead, and keystrokes may be interpreted as menu navigation rather than normal text/input interaction). [1][2][3]
If you need a “share panel” (multiple interactive elements, form fields, etc.), use a Popover/Dialog-style component rather than Menu (or open a dialog from a Menu.Item, as shown in the Menu docs). [3]
Sources:
[1] Base UI MenuRoot implementation (role="menu", useListNavigation, useTypeahead, useDismiss) (app.unpkg.com)
[2] Base UI useMenuItem implementation (role="menuitem", roving tabIndex) (app.unpkg.com)
[3] Base UI Menu docs (“list of actions… keyboard navigation”; example opening a dialog from a menu item) (base-ui.com)
Citations:
- 1: https://app.unpkg.com/%40base-ui/react%401.1.0/files/menu/root/MenuRoot.js
- 2: https://app.unpkg.com/%40base-ui/react%401.1.0/files/menu/item/useMenuItem.js
- 3: https://base-ui.com/react/components/menu?utm_source=openai
🏁 Script executed:
# First, let's find and examine the ShareContent component
fd "ShareContent" --type fRepository: timelessco/recollect
Length of output: 46
🏁 Script executed:
# Also check the full context of the collection-options-popover.tsx file
cat -n src/pageComponents/dashboard/sidePane/collection-options-popover.tsx | head -130Repository: timelessco/recollect
Length of output: 4930
🏁 Script executed:
# Find and examine the ShareContent component
find . -name "shareContent*" -o -name "share-content*" | head -20Repository: timelessco/recollect
Length of output: 117
🏁 Script executed:
# Also check for the file by searching imports
rg "ShareContent" -A 2 --type tsx | head -50Repository: timelessco/recollect
Length of output: 91
🏁 Script executed:
# Examine the ShareContent component
cat -n src/pageComponents/dashboard/share/shareContent.tsx | head -100Repository: timelessco/recollect
Length of output: 4374
🏁 Script executed:
# Get more of the ShareContent component to understand its full structure
cat -n src/pageComponents/dashboard/share/shareContent.tsx | tail -200Repository: timelessco/recollect
Length of output: 7037
🏁 Script executed:
# Check what SharePublicSwitch contains - this might be interactive
cat -n src/pageComponents/dashboard/share/share-public-switch.tsxRepository: timelessco/recollect
Length of output: 2303
🏁 Script executed:
# Check if Base UI Popover is used elsewhere in the codebase
rg "Popover|popover" src/ --type ts | head -20Repository: timelessco/recollect
Length of output: 1776
🏁 Script executed:
# Check the Menu component wrapper to understand its structure
cat -n src/components/ui/recollect/menu.tsx | head -50Repository: timelessco/recollect
Length of output: 2096
Replace Menu.Popup with a neutral popover for the share view.
Menu.Popup applies role="menu" with composite keyboard navigation (roving focus, arrow keys, typeahead across Menu.Item descendants), but the share branch renders arbitrary interactive content (form inputs, dropdowns, switches, buttons) that cannot participate in menu semantics. This breaks keyboard navigation and produces incorrect screen-reader output.
Use the Popover component (src/components/ui/recollect/popover.tsx) instead for the share panel—it's the correct primitive for non-menu interactive content, and the component is already available in the codebase.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pageComponents/dashboard/sidePane/collection-options-popover.tsx` around
lines 68 - 119, The share panel currently renders inside Menu.Popup (see
Menu.Popup usage and the view === "share" branch that renders <ShareContent
categoryId={item.id} />), which forces menu semantics; replace that branch so
the "share" view uses the Popover primitive (the Popover component used
elsewhere in the app) instead of Menu.Popup: move the share panel out of
Menu.Popup or swap the wrapper only for the share branch to render a
Popover.Positioner/Popover.Content (or equivalent Popover API in your codebase)
that contains the same ShareContent markup, ensuring Menu.Item remains for the
"menu" view and keyboard/screen-reader semantics are correct.
1 participant
PR Checklist
Overview
Summary by CodeRabbit
Style
Refactor
New Features