-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implemented more button in TabList #10096
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
This PR implements a "More" dropdown button in the TabList component, replacing the scrolling mechanism for handling overflow tabs with a cleaner dropdown interface.
- Added new
TabMoreButton
component in/packages/twenty-front/src/modules/ui/layout/tab/components/TabMoreButton.tsx
with dropdown functionality and styled hover states - Implemented
MAX_VISIBLE_TABS
constant (4) in/packages/twenty-front/src/modules/ui/layout/tab/components/TabList.tsx
with logic to split tabs into visible and overflow sections - Added
inDropdown
prop to Tab component for consistent styling when rendered in dropdown menu - Enhanced hover states and border radius styling for dropdown menu items
- Integrated hotkey scope management for dropdown accessibility
💡 (2/5) Greptile learns from your feedback when you react with 👍/👎!
3 file(s) reviewed, 6 comment(s)
Edit PR Review Bot Settings | Greptile
packages/twenty-front/src/modules/ui/layout/tab/components/TabMoreButton.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/ui/layout/tab/components/TabMoreButton.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/ui/layout/tab/components/TabMoreButton.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/ui/layout/tab/components/Tab.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/ui/layout/tab/components/TabList.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/ui/layout/tab/components/TabList.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @atharvParlikar :)
I have not tried your code locally as the ci is failing, mostly due to lint.
However I left some comments on the code to improve on it.
tip: run yarn nx run twenty-front:lint
to check lint issues
and yarn nx run twenty-front:lint:fix
to potentially fix them
packages/twenty-front/src/modules/ui/layout/tab/components/TabMoreButton.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/ui/layout/tab/components/TabMoreButton.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/ui/layout/tab/components/TabMoreButton.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/ui/layout/tab/components/TabMoreButton.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/ui/layout/tab/components/TabMoreButton.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/ui/layout/tab/components/TabList.tsx
Outdated
Show resolved
Hide resolved
|
||
const initialActiveTabId = activeTabId || visibleTabs[0]?.id || ''; | ||
|
||
useEffect(() => { | ||
setActiveTabId(initialActiveTabId); | ||
}, [initialActiveTabId, setActiveTabId]); | ||
}, []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
During development there was an issue of infinite re-rendering, so I removed it for the time being but it seems I forgot to put these back after fixing it...
id="more-button" | ||
title={`+${remainingTabs.length} More`} | ||
Icon={IconChevronDown} | ||
dropdownContent={ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of passing dropdownContent as a react node why not just pass remaining tab and use them in Dropdown ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not sure about this. Since Tab is already rendered in the parent component of MoreButton, it has all the necessary state for rendering tabs. Some props (like activeTabId, behaveAsLinks, and handleTabClick) need to be passed down to MoreButton.
If we render the tabs inside MoreButton, it adds complexity, whereas passing dropdownContent as a ReactNode keeps it simpler. What do you think?
packages/twenty-front/src/modules/ui/layout/tab/components/TabMoreButton.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/ui/layout/tab/components/Tab.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
(updates since last review)
This PR continues the implementation of the "More" dropdown button in TabList with additional refinements and fixes.
- Fixed StyledButtonContainer definition by moving it outside the render function in
/packages/twenty-front/src/modules/ui/layout/tab/components/TabMoreDropdown.tsx
- Added proper IconPosition support in Button component for right-aligned chevron in
/packages/twenty-ui/src/input/button/components/Button.tsx
- Implemented consistent hover states for tabs in dropdown using theme colors in
/packages/twenty-front/src/modules/ui/layout/tab/components/Tab.tsx
5 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile
))} | ||
{remainingTabs.length > 0 && ( | ||
<MoreTabsDropdown | ||
id="more-button" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider making 'more-button' id unique by appending tabListInstanceId to prevent potential conflicts with multiple tab lists
onDropdownClose, | ||
}: MoreTabsDropdownProps) => { | ||
if (!dropdownContent) { | ||
return <Button title={title} Icon={Icon} />; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: missing variant and size props here to match the dropdown button styling
<Button | ||
variant="tertiary" | ||
size="small" | ||
title={title} | ||
Icon={Icon} | ||
IconPosition="right" | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
syntax: IconPosition prop is not defined in Button component types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a comment, if you need any help with this PR do tag me :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think MAX_VISIBLE_TABS
matches the desired behavior.
The desired behavior suggests taking a different approach, as shown in the screenshot.

As per @lucasbordeau's comment, we should implement this approach.
While your current implementation might work, it also constrains tabs from being more dynamic. Would you be able to look into this alternative approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, true.
I was only looking at that particular example, didn't think about if tabs can be used elsewhere.
I'll refactor it to match Lucas's suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made the changes @ehconitin can review it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
(updates since last review)
This PR continues the implementation of the "More" dropdown button in TabList with significant improvements to the dynamic tab calculation and styling.
- Added dynamic tab width calculation in
TabList.tsx
usinguseEffect
to determine maximum visible tabs based on container and tab widths - Improved dropdown styling in
Tab.tsx
with proper theme-based hover states and border radius for consistent appearance - Created new
TabMoreDropdown.tsx
component with proper dropdown placement and hotkey scope management - Removed
tabList
context fromScrollWrapperContexts.tsx
as part of migration away from scroll behavior - Added
IconPosition
prop support inButton.tsx
for right-aligned chevron icon
The implementation now properly handles overflow tabs with dynamic calculations rather than a fixed limit, providing a more flexible solution.
5 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
const firstTab = containerRef.current.querySelector( | ||
'.tab-item', | ||
) as HTMLElement; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: firstTab could be null if no tabs are rendered yet, causing a runtime error
const firstTab = containerRef.current.querySelector( | |
'.tab-item', | |
) as HTMLElement; | |
const firstTab = containerRef.current.querySelector( | |
'.tab-item', | |
) as HTMLElement | null; | |
if (!firstTab) return; |
const StyledButtonContainer = styled.div` | ||
height: 100%; | ||
display: flex; | ||
align-items: center; | ||
`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: StyledButtonContainer should be defined outside the component to prevent recreation on each render
const StyledButtonContainer = styled.div` | |
height: 100%; | |
display: flex; | |
align-items: center; | |
`; | |
const StyledButtonContainer = styled.div` | |
height: 100%; | |
display: flex; | |
align-items: center; | |
`; | |
export const MoreTabsDropdown = ({ |
); | ||
const containerRef = useRef<HTMLDivElement>(null); | ||
|
||
useEffect(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you find another way than using a useEffect ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the concern is repainting the UI, we could use useLayoutEffect instead of useEffect. Or are you suggesting avoiding effects altogether?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
later
fixes: #9904
Implemented a more button in TabList, replacing the Scroll mechanism,