feat(sidebar-controls): added ability to expand on hover#343
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Added hover mode functionality to the sidebar, allowing dynamic expansion on hover alongside existing expanded/collapsed states, with new controls for mode selection and improved workspace header interactions.
- Added
sidebar-control.tsxwith a Popover menu for selecting between expanded, collapsed, and hover modes - Updated
store.tsto replace simple collapsed state with mode-based system (expanded,collapsed,hover) - Added z-index management in
globals.cssfor proper sidebar overlay layering (z-index: 40) - Modified
workflow.tsxto handle sidebar state transitions withisSidebarCollapsed = mode === 'expanded' ? !isExpanded : mode === 'collapsed' || mode === 'hover' - Added event propagation controls in
workspace-header.tsxto prevent unintended sidebar collapse in hover mode
💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!
8 file(s) reviewed, 11 comment(s)
Edit PR Review Bot Settings | Greptile
| const isSidebarCollapsed = | ||
| mode === 'expanded' ? !isExpanded : mode === 'collapsed' || mode === 'hover' |
There was a problem hiding this comment.
logic: Potential edge case: isSidebarCollapsed will be true in hover mode even when hovering (expanded). This may cause toolbar position jumps during hover.
| onClick={() => setIsCollapsed(false)} | ||
| className={`fixed transition-left duration-200 ${isSidebarCollapsed ? 'left-20' : 'left-64'} bottom-[18px] z-10 flex h-9 w-9 items-center justify-center rounded-lg bg-background text-muted-foreground hover:text-foreground hover:bg-accent border`} | ||
| onClick={() => setIsToolbarOpen(true)} | ||
| className={`fixed transition-all duration-200 ${isSidebarCollapsed ? 'left-20' : 'left-64'} bottom-[18px] z-10 flex h-9 w-9 items-center justify-center rounded-lg bg-background text-muted-foreground hover:text-foreground hover:bg-accent border`} |
There was a problem hiding this comment.
style: transition-all may cause unnecessary transitions on unrelated properties. Consider using specific transition properties like 'transition-[left]'
| .main-content-overlay:hover { | ||
| box-shadow: 0 10px 15px -3px rgba(0, 0, 0, 0.1), 0 4px 6px -2px rgba(0, 0, 0, 0.05); | ||
| } |
There was a problem hiding this comment.
style: box-shadow values should use CSS custom properties for consistency with the design system and dark mode support
| const handleModeChange = (value: SidebarMode) => { | ||
| // When selecting expanded mode, ensure it's expanded | ||
| if (value === 'expanded' && !isExpanded) { | ||
| toggleExpanded() | ||
| } | ||
|
|
||
| // Set the new mode | ||
| setMode(value) | ||
| setOpen(false) | ||
| } |
There was a problem hiding this comment.
style: Consider adding a check to prevent unnecessary state updates when selecting the current mode
| sideOffset={5} | ||
| > | ||
| <div className="border-b py-[10px] px-4"> | ||
| <h4 className="text-xs font-[480] text-muted-foreground">Sidebar control</h4> |
There was a problem hiding this comment.
style: font-[480] is an unusual font weight value. Consider using standard weights like 400 or 500
| useEffect(() => { | ||
| if (isAnyModalOpen) { | ||
| setExplicitMouseEnter(false) | ||
| } | ||
| }, [isAnyModalOpen]) |
There was a problem hiding this comment.
logic: Resetting explicitMouseEnter when modal opens could cause sidebar to unexpectedly collapse while user is still hovering
| mode: SidebarMode | ||
| isExpanded: boolean |
There was a problem hiding this comment.
style: Having both mode and isExpanded could lead to inconsistent states. Consider using only mode and deriving expanded state from it.
| setMode: (mode) => set({ mode }), | ||
| toggleExpanded: () => set((state) => ({ isExpanded: !state.isExpanded })), |
There was a problem hiding this comment.
logic: setMode should also update isExpanded to maintain state consistency (e.g. when mode is 'collapsed', isExpanded should be false)
| // Keep local isOpen state in sync with the store (for internal component use) | ||
| const [isOpen, setIsOpen] = useState(workspaceDropdownOpen) |
There was a problem hiding this comment.
logic: local isOpen state initialized from workspaceDropdownOpen but no sync effect to keep them in sync
| // Keep local isOpen state in sync with the store (for internal component use) | |
| const [isOpen, setIsOpen] = useState(workspaceDropdownOpen) | |
| // Keep local isOpen state in sync with the store (for internal component use) | |
| const [isOpen, setIsOpen] = useState(workspaceDropdownOpen) | |
| useEffect(() => { | |
| setIsOpen(workspaceDropdownOpen) | |
| }, [workspaceDropdownOpen]) |
| // Special handling for click interactions in hover mode | ||
| const handleTriggerClick = (e: React.MouseEvent) => { | ||
| // When in hover mode, explicitly prevent bubbling for the trigger | ||
| if (mode === 'hover') { | ||
| e.stopPropagation() | ||
| e.preventDefault() | ||
| // Toggle dropdown state | ||
| handleDropdownOpenChange(!isOpen) | ||
| } | ||
| } |
There was a problem hiding this comment.
logic: handleTriggerClick only toggles dropdown in hover mode but should also handle click mode
2fc1260 to
88e9634
Compare
88e9634 to
e76add0
Compare
1 participant
Description
Extended sidebar capability to expand on hover in addition to expand/collapse.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Open/close toolbar, workflow, and logs. Set different sidebar controls and test out each page.
Checklist:
npm test)Security Considerations: