-
Notifications
You must be signed in to change notification settings - Fork 4.2k
feat: new tab list #12384
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
feat: new tab list #12384
Conversation
a compoenent inside a component
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:35747 This environment will automatically shut down when the PR is closed or after 5 hours. |
@greptileai trigger |
disabled: !canAccessFullAdminPanel, | ||
}, | ||
{ | ||
id: SETTINGS_ADMIN_TABS.CHECK_1, |
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.
temporary seeds
return <SettingsAdminConfigVariables />; | ||
case SETTINGS_ADMIN_TABS.HEALTH_STATUS: | ||
return <SettingsAdminHealthStatus />; | ||
case SETTINGS_ADMIN_TABS.CHECK_1: |
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.
same
CHECK_5: 'check-5', | ||
CHECK_6: 'check-6', | ||
CHECK_7: 'check-7', | ||
CHECK_8: 'check-8', |
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.
same
Thanks for sharing @ehconitin CurrentDesiredCan you gather the "More" tab with the other tabs? Also can you correct the chevron down icon color, font-weight & spacing? |
@greptileai trigger |
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
Implements a new tab list feature replacing scrollable tabs with a '+X more' dropdown for overflow tabs, following the design specifications from Figma.
- Remove test tabs (CHECK_1 through CHECK_8) from
SettingsAdminTabs.ts
andSettingsAdminTabContent.tsx
as they appear to be temporary development artifacts - Consider adding accessibility attributes (aria-labels, roles) to
TabMoreButton
andTabListDropdown
components for better screen reader support - Add error boundaries around the tab overflow detection logic in
isFirstOverflowingTab.ts
to handle edge cases when elements are null - Consider adding ResizeObserver to handle dynamic width changes in
TabList
component - Add unit tests for the overflow calculation logic to ensure consistent behavior across different screen sizes
13 file(s) reviewed, 12 comment(s)
Edit PR Review Bot Settings | Greptile
packages/twenty-front/src/modules/settings/admin-panel/components/SettingsAdminContent.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/settings/admin-panel/components/SettingsAdminContent.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/settings/admin-panel/components/SettingsAdminTabContent.tsx
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/settings/admin-panel/constants/SettingsAdminTabs.ts
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/ui/layout/tab/components/TabAvatar.tsx
Show resolved
Hide resolved
packages/twenty-front/src/modules/ui/layout/tab/components/TabListDropdown.tsx
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/TabSharedStyles.tsx
Outdated
Show resolved
Hide resolved
...ages/twenty-front/src/modules/ui/layout/tab/components/__stories__/TabMoreButton.stories.tsx
Show resolved
Hide resolved
packages/twenty-front/src/modules/ui/layout/tab/utils/isFirstOverflowingTab.ts
Outdated
Show resolved
Hide resolved
packages/twenty-front/src/modules/ui/layout/tab/components/TabAvatar.tsx
Show resolved
Hide resolved
</StyledTab> | ||
); | ||
}; | ||
Tab.displayName = 'Tab'; |
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 do we need to add this here? Is it because of the forwardRef?
Seems non standard to me but we can revisit if there is a good reason
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.
Yes forward ref! Not related to this PR.
pretty sure this was added just for clear debugging in devtools
packages/twenty-front/src/modules/ui/layout/tab/components/TabList.tsx
Outdated
Show resolved
Hide resolved
resetFirstHiddenTabIndex, | ||
]); | ||
|
||
const handleTabSelectFromDropdown = useCallback( |
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.
then you start using callbacks because you have re-renders because of useEffects
packages/twenty-front/src/modules/ui/layout/tab/components/TabListDropdown.tsx
Show resolved
Hide resolved
packages/twenty-front/src/modules/ui/layout/tab/components/TabSharedStyles.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.
Hi @ehconitin, thanks for taking this one
I'm not a fan of the current approach which is relying on refs and effects and is not re-usable for similar problems in the code base.
A different approach:
- re-introduce a component that will render the child component without displaying it (visibility: hidden) => this component will contains a useEffect indeed
- render all the tabs within NodeDimension and onDimensionChange increment a counter (you'll likely need to store all tab size in an array and take the sum
- you can also leverage onDimensionChange callback to get the width of the container in a state
- then you can easily compute how many tabs you can fit in the container
packages/twenty-front/src/modules/ui/utilities/dimensions/components/NodeDimension.tsx
Show resolved
Hide resolved
...kflow-steps/workflow-actions/code-action/components/WorkflowEditActionServerlessFunction.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.
Looks way better, let's fix the comments and the tests :)
return visibleTabs.length; | ||
} | ||
|
||
const availableWidth = containerWidth - TAB_LIST_LEFT_PADDING; |
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.
@charlesBochet
what should I do here? This padding isn't the case everytime, it is being styled in the parent to add paddings.
we could pass in padding as a prop but its not standard!
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.
Indeed! We could get the "insideContainerWidth" by rendering a width:100% div within the container and using NodeDimension in this case
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.
TY @ehconitin for completing this and taking all the feedbacks
I think we can merge it as it is. If you have time, there are still two areas we could improve:
- simplify the containerWidth calculation: I'm not sure we need to render all the children, a width:100% should suffice (or anything that would take all the width)
- take padding into account indeed, it seems that we have a bit more room on show page tabs
Will do! |
closes #9904 --------- Co-authored-by: Charles Bochet <[email protected]>
closes twentyhq#9904 --------- Co-authored-by: Charles Bochet <[email protected]>
closes #9904