Skip to content

Implemented more button in TabList #10096

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

Closed
wants to merge 13 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions packages/twenty-front/src/modules/ui/layout/tab/components/Tab.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ type TabProps = {
title: string;
Icon?: IconComponent;
active?: boolean;
inDropdown?: boolean;
className?: string;
onClick?: () => void;
disabled?: boolean;
Expand All @@ -21,7 +22,7 @@ type TabProps = {

const StyledTab = styled('button', {
shouldForwardProp: (prop) => isPropValid(prop) && prop !== 'active',
})<{ active?: boolean; disabled?: boolean; to?: string }>`
})<{ active?: boolean; disabled?: boolean; to?: string; inDropdown?: boolean }>`
align-items: center;
border-bottom: 1px solid ${({ theme }) => theme.border.color.light};
border-color: ${({ theme, active }) =>
Expand All @@ -43,8 +44,14 @@ const StyledTab = styled('button', {
justify-content: center;
margin-bottom: 0;
padding: ${({ theme }) => theme.spacing(2) + ' 0'};
pointer-events: ${({ disabled }) => (disabled ? 'none' : '')};
pointer-events: ${({ disabled }) => (disabled ? 'none' : 'auto')};
text-decoration: none;

&:hover {
background: ${({ theme, inDropdown }) =>
inDropdown ? theme.background.tertiary : 'transparent'};
}
border-radius: ${({ theme }) => theme.border.radius.sm};
`;

const StyledHover = styled.span`
Expand Down Expand Up @@ -73,6 +80,7 @@ export const Tab = ({
title,
Icon,
active = false,
inDropdown = false,
onClick,
className,
disabled,
Expand All @@ -91,6 +99,7 @@ export const Tab = ({
active={active}
className={className}
disabled={disabled}
inDropdown={inDropdown}
data-testid={'tab-' + id}
as={to ? Link : 'button'}
to={to}
Expand Down
117 changes: 83 additions & 34 deletions packages/twenty-front/src/modules/ui/layout/tab/components/TabList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,11 @@ import { TabListFromUrlOptionalEffect } from '@/ui/layout/tab/components/TabList
import { useTabList } from '@/ui/layout/tab/hooks/useTabList';
import { TabListScope } from '@/ui/layout/tab/scopes/TabListScope';
import { LayoutCard } from '@/ui/layout/tab/types/LayoutCard';
import { ScrollWrapper } from '@/ui/utilities/scroll/components/ScrollWrapper';
import styled from '@emotion/styled';
import * as React from 'react';
import { useEffect } from 'react';
import { IconComponent } from 'twenty-ui';
import { useEffect, useState, useCallback } from 'react';
import { Tab } from './Tab';
import { MoreTabsDropdown } from './TabMoreDropdown';
import { IconComponent, IconChevronDown } from 'twenty-ui';

export type SingleTabProps = {
title: string;
Expand Down Expand Up @@ -38,17 +37,41 @@ const StyledContainer = styled.div`
user-select: none;
`;

const StyledDropdownContainer = styled.div`
display: flex;
flex-direction: column;
padding: ${({ theme }) => theme.spacing(1)};
`;

export const TabList = ({
tabs,
tabListInstanceId,
loading,
loading = false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can leave it as it was it defaults to false

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2025-02-16 at 3 17 02 AM

It seems it defaults to undefined

behaveAsLinks = true,
isInRightDrawer,
isInRightDrawer = false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above comment

className,
}: TabListProps) => {
const { activeTabId, setActiveTabId } = useTabList(tabListInstanceId);
const visibleTabs = tabs.filter((tab) => !tab.hide);

const { activeTabId, setActiveTabId } = useTabList(tabListInstanceId);
const [maxVisibleTabs, setMaxVisibleTabs] = useState<number>(
visibleTabs.length,
);
Comment on lines +57 to +59
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Initial maxVisibleTabs state could cause layout shift. Consider setting to 0 initially and updating after measurement.

Suggested change
const [maxVisibleTabs, setMaxVisibleTabs] = useState<number>(
visibleTabs.length,
);
const [maxVisibleTabs, setMaxVisibleTabs] = useState<number>(0);


const containerRef = (node: HTMLDivElement) => {
const containerWidth = node.offsetWidth;
const firstTab = node.querySelector(
'.tab-item',
) as HTMLElement;
if (!firstTab) return;

const tabWidth = firstTab.offsetWidth + 16; // 16px := gap between tabs
const calculatedMaxVisible = Math.floor(containerWidth / tabWidth) - 1; // -1 to make space for the dropdown button
setMaxVisibleTabs(calculatedMaxVisible);
};
Comment on lines +61 to +71
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: containerRef callback will re-run on every render. Consider using useCallback to prevent unnecessary recalculations.


const truncatedTabs = visibleTabs.slice(0, maxVisibleTabs);
const remainingTabs = visibleTabs.slice(maxVisibleTabs);

const initialActiveTabId = activeTabId || visibleTabs[0]?.id || '';

Expand All @@ -60,39 +83,65 @@ export const TabList = ({
return null;
}

const handleTabClick = (tabId: string) => {
if (!behaveAsLinks) {
setActiveTabId(tabId);
}
};

return (
<TabListScope tabListScopeId={tabListInstanceId}>
<TabListFromUrlOptionalEffect
isInRightDrawer={!!isInRightDrawer}
isInRightDrawer={isInRightDrawer}
componentInstanceId={tabListInstanceId}
tabListIds={tabs.map((tab) => tab.id)}
/>
<ScrollWrapper
defaultEnableYScroll={false}
contextProviderName="tabList"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! if we are removing this contextProvider from ScrollWrapper may be check if its being used else where, if not remove the context from scrollwrapper contexts

componentInstanceId={`scroll-wrapper-tab-list-${tabListInstanceId}`}
>
<StyledContainer className={className}>
{visibleTabs.map((tab) => (
<Tab
id={tab.id}
key={tab.id}
title={tab.title}
Icon={tab.Icon}
logo={tab.logo}
active={tab.id === activeTabId}
disabled={tab.disabled ?? loading}
pill={tab.pill}
to={behaveAsLinks ? `#${tab.id}` : undefined}
onClick={() => {
if (!behaveAsLinks) {
setActiveTabId(tab.id);
}
}}
/>
))}
</StyledContainer>
</ScrollWrapper>
<StyledContainer className={className} ref={containerRef}>
{truncatedTabs.map((tab) => (
<Tab
key={tab.id}
id={tab.id}
title={tab.title}
Icon={tab.Icon}
logo={tab.logo}
active={tab.id === activeTabId}
disabled={tab.disabled ?? loading}
pill={tab.pill}
to={behaveAsLinks ? `#${tab.id}` : undefined}
onClick={() => handleTabClick(tab.id)}
inDropdown={false}
// Add a class for measurement
className="tab-item"
/>
))}
{remainingTabs.length > 0 && (
<MoreTabsDropdown
id="more-button"
Copy link
Contributor

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

title={`+${remainingTabs.length} More`}
Icon={IconChevronDown}
dropdownContent={
Copy link
Contributor

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 ?

Copy link
Contributor Author

@atharvParlikar atharvParlikar Feb 15, 2025

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?

<StyledDropdownContainer>
{remainingTabs.map((tab) => (
<Tab
key={tab.id}
id={tab.id}
title={tab.title}
Icon={tab.Icon}
logo={tab.logo}
active={tab.id === activeTabId}
disabled={tab.disabled ?? loading}
pill={tab.pill}
to={behaveAsLinks ? `#${tab.id}` : undefined}
onClick={() => handleTabClick(tab.id)}
inDropdown={true}
/>
))}
</StyledDropdownContainer>
}
dropdownPlacement="bottom-start"
/>
)}
</StyledContainer>
</TabListScope>
);
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
import { Button, IconComponent } from 'twenty-ui';
import { HotkeyScope } from '@/ui/utilities/hotkey/types/HotkeyScope';
import styled from '@emotion/styled';
import { Dropdown } from '@/ui/layout/dropdown/components/Dropdown';

type MoreTabsDropdownProps = {
id: string;
title: string;
Icon: IconComponent;
dropdownContent?: React.ReactNode;
dropdownPlacement?: 'bottom-start' | 'bottom-end' | 'top-start' | 'top-end';
onDropdownOpen?: () => void;
onDropdownClose?: () => void;
};

export const MoreTabsDropdown = ({
id,
title,
Icon,
dropdownContent,
dropdownPlacement = 'bottom-start',
onDropdownOpen,
onDropdownClose,
}: MoreTabsDropdownProps) => {
if (!dropdownContent) {
return <Button title={title} Icon={Icon} />;
Copy link
Contributor

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

}

const StyledButtonContainer = styled.div`
height: 100%;
display: flex;
align-items: center;
`;
Comment on lines +29 to +33
Copy link
Contributor

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

Suggested change
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 = ({


// Define a basic hotkey scope for the dropdown
const dropdownHotkeyScope: HotkeyScope = {
scope: `dropdown-${id}`,
};

return (
<Dropdown
dropdownId={`more-button-${id}-dropdown`}
dropdownHotkeyScope={dropdownHotkeyScope}
dropdownPlacement={dropdownPlacement}
dropdownComponents={dropdownContent}
onOpen={onDropdownOpen}
onClose={onDropdownClose}
clickableComponent={
<StyledButtonContainer>
<Button
variant="tertiary"
size="small"
title={title}
Icon={Icon}
IconPosition="right"
/>
Comment on lines +50 to +56
Copy link
Contributor

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

</StyledButtonContainer>
}
/>
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ export type ContextProviderName =
| 'dropdownMenuItemsContainer'
| 'showPageContainer'
| 'showPageLeftContainer'
| 'tabList'
| 'releases'
| 'test'
| 'showPageActivityContainer'
Expand Down Expand Up @@ -44,8 +43,6 @@ export const ShowPageContainerScrollWrapperContext =
createScrollWrapperContext('showPageContainer');
export const ShowPageLeftContainerScrollWrapperContext =
createScrollWrapperContext('showPageLeftContainer');
export const TabListScrollWrapperContext =
createScrollWrapperContext('tabList');
export const ReleasesScrollWrapperContext =
createScrollWrapperContext('releases');
export const ShowPageActivityContainerScrollWrapperContext =
Expand Down Expand Up @@ -78,8 +75,6 @@ export const getContextByProviderName = (
return ShowPageContainerScrollWrapperContext;
case 'showPageLeftContainer':
return ShowPageLeftContainerScrollWrapperContext;
case 'tabList':
return TabListScrollWrapperContext;
case 'releases':
return ReleasesScrollWrapperContext;
case 'test':
Expand Down
6 changes: 5 additions & 1 deletion packages/twenty-ui/src/input/button/components/Button.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,12 @@ export type ButtonSize = 'medium' | 'small';
export type ButtonPosition = 'standalone' | 'left' | 'middle' | 'right';
export type ButtonVariant = 'primary' | 'secondary' | 'tertiary';
export type ButtonAccent = 'default' | 'blue' | 'danger';
export type IconPosition = 'left' | 'right';

export type ButtonProps = {
className?: string;
Icon?: IconComponent;
IconPosition?: IconPosition;
title?: string;
fullWidth?: boolean;
variant?: ButtonVariant;
Expand Down Expand Up @@ -398,6 +400,7 @@ const StyledShortcutLabel = styled.div<{
export const Button = ({
className,
Icon,
IconPosition = 'left',
title,
fullWidth = false,
variant = 'primary',
Expand Down Expand Up @@ -445,8 +448,9 @@ export const Button = ({
onFocus={() => setIsFocused(true)}
onBlur={() => setIsFocused(false)}
>
{Icon && <Icon size={theme.icon.size.sm} />}
{Icon && IconPosition === 'left' && <Icon size={theme.icon.size.sm} />}
{title}
{Icon && IconPosition === 'right' && <Icon size={theme.icon.size.sm} />}
{hotkeys && !isMobile && (
<>
<StyledSeparator buttonSize={size} accent={accent} />
Expand Down
Loading