feat: navigate tab #219
feat: navigate tab #219
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds horizontal scroll navigation to the results tabs interface. When there are many tabs, users can now use left and right chevron buttons to scroll through them instead of relying on native browser scrolling.
Key changes:
- Implements scroll buttons with chevron icons for navigating tabs horizontally
- Adds state management to show/hide scroll buttons based on scroll position
- Restructures the tab bar layout to support absolute-positioned scroll buttons
| <button | ||
| type="button" | ||
| onClick={() => scroll('right')} | ||
| className="absolute right-0 z-10 flex items-center justify-center w-6 h-full bg-background hover:bg-muted cursor-pointer" | ||
| aria-label="Scroll tabs right" | ||
| > | ||
| <ChevronRightIcon className="w-4 h-4 text-muted-foreground" /> | ||
| </button> |
There was a problem hiding this comment.
The scroll buttons lack keyboard accessibility. The buttons can only be activated by mouse clicks, but users navigating with a keyboard cannot use them effectively. While the buttons have type="button" and aria-label attributes, they should also be properly focusable and respond to keyboard events (Enter and Space keys) to ensure full accessibility compliance.
| const scroll = (direction: 'left' | 'right') => { | ||
| const container = tabsContainerRef.current; | ||
| if (!container) return; | ||
|
|
||
| const scrollAmount = 150; | ||
| container.scrollBy({ | ||
| left: direction === 'left' ? -scrollAmount : scrollAmount, | ||
| behavior: 'smooth', | ||
| }); | ||
| }; |
There was a problem hiding this comment.
The scroll callback does not trigger updateScrollButtons after scrolling completes. When using smooth scrolling behavior, the scroll position updates asynchronously, but updateScrollButtons is only called during onScroll events. This can cause the scroll buttons to not update their visibility state immediately after clicking them, leading to a poor user experience where buttons remain visible when they shouldn't be or vice versa.
| ref={tabsContainerRef} | ||
| onScroll={updateScrollButtons} | ||
| className={cn( | ||
| "flex items-center gap-1 overflow-hidden", |
There was a problem hiding this comment.
The tabs container has overflow-hidden which prevents the container from being scrollable. For the scroll function to work (which calls container.scrollBy()), the element must have a scrollable overflow context. The container should use overflow-x-auto or overflow-x-scroll with scrollbar-hidden styling (e.g., via CSS or Tailwind classes like scrollbar-hide) to enable scrolling while hiding the scrollbar visually.
| "flex items-center gap-1 overflow-hidden", | |
| "flex items-center gap-1 overflow-x-auto overflow-y-hidden", |
| function ChevronLeftIcon({ className }: { className?: string }) { | ||
| return ( | ||
| <svg className={className} fill="none" viewBox="0 0 24 24" stroke="currentColor" strokeWidth={2}> | ||
| <path strokeLinecap="round" strokeLinejoin="round" d="M15 19l-7-7 7-7" /> | ||
| </svg> | ||
| ); | ||
| } | ||
|
|
||
| function ChevronRightIcon({ className }: { className?: string }) { | ||
| return ( | ||
| <svg className={className} fill="none" viewBox="0 0 24 24" stroke="currentColor" strokeWidth={2}> | ||
| <path strokeLinecap="round" strokeLinejoin="round" d="M9 5l7 7-7 7" /> | ||
| </svg> | ||
| ); | ||
| } |
There was a problem hiding this comment.
The chevron icon components are defined inline but follow a different pattern than the existing icon components in the codebase. According to the pattern established by XIcon, CheckIcon, and other icons in frontend/src/components/icons/, icon components should be exported as default exports in separate files with proper TypeScript interfaces and default className values. Consider moving these icons to separate files in the icons directory to maintain consistency.
| const container = tabsContainerRef.current; | ||
| if (!container) return; | ||
|
|
||
| const scrollAmount = 150; |
There was a problem hiding this comment.
The magic number 150 for scroll amount lacks context and makes the scrolling behavior difficult to adjust. Consider extracting this to a named constant (e.g., SCROLL_AMOUNT or TAB_SCROLL_DISTANCE) at the top of the component or file to improve code maintainability and make the purpose clearer.
| <button | ||
| key={tab.id} | ||
| type="button" | ||
| onClick={() => onTabSelect(tab.id)} | ||
| className={cn( | ||
| 'group flex items-center gap-1.5 px-3 py-1.5 text-sm whitespace-nowrap', | ||
| 'border-b-2 -mb-px transition-colors cursor-pointer', | ||
| tab.id === activeTabId | ||
| ? 'border-primary text-foreground' | ||
| : 'border-transparent text-muted-foreground hover:text-foreground' | ||
| )} | ||
| onClick={() => scroll('left')} | ||
| className="absolute left-0 z-10 flex items-center justify-center w-6 h-full bg-background hover:bg-muted cursor-pointer" | ||
| aria-label="Scroll tabs left" | ||
| > | ||
| <span>{formatTimestamp(tab.timestamp)}</span> | ||
| {tab.error && ( | ||
| <span className="w-1.5 h-1.5 rounded-full bg-destructive" aria-label="Error" /> | ||
| )} | ||
| <span | ||
| role="button" | ||
| tabIndex={0} | ||
| aria-label="Close tab" | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| onTabClose(tab.id); | ||
| }} | ||
| onKeyDown={(e) => { | ||
| if (e.key === 'Enter' || e.key === ' ') { | ||
| <ChevronLeftIcon className="w-4 h-4 text-muted-foreground" /> | ||
| </button> |
There was a problem hiding this comment.
The scroll buttons lack keyboard accessibility. The buttons can only be activated by mouse clicks, but users navigating with a keyboard cannot use them effectively. While the buttons have type="button" and aria-label attributes, they should also be properly focusable and respond to keyboard events (Enter and Space keys) to ensure full accessibility compliance.
No description provided.