Hide tabs #7841
Hide tabs #7841
Conversation
|
Discussed in DM - It works but it would be cool to move to progressively to something a bit more abstract, like that
|
|
Let's aim to close this soon as it's been open for 2 weeks now! Or maybe close the PR and re-open it when it's ready? |
|
@FelixMalfait for now I have shoved everything into a single file --- but how about this? :) |
| loading || baseHide || objectsDontExist || relationsDontExist; | ||
|
|
||
| // Special case handling from original code | ||
| if (!isHidden) { |
There was a problem hiding this comment.
Hey, this is a good step forward but I was hoping you'd get rid of most of this code!
The goal is not to have to create
case 'Note':
return isNotesObjectActive;
But instead directly pass CoreObjectNameSingular.Note etc
| CoreObjectNameSingular.WorkflowRun, | ||
| ].includes(obj), | ||
| ), | ||
| }, | ||
| }, | ||
| { | ||
| id: 'emails', |
There was a problem hiding this comment.
Let's transform this array to an object and use the ID as the key of the array, this will make it easier to over write layouts. Let's maybe do the same for the Cards.
(for inheritance then we can quickly overwrite a given tab/card)
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR refactors tab visibility and card rendering logic by introducing a more structured and maintainable approach using configuration-based layouts and centralized component mapping.
- Added
CardComponents.tsxto centralize card rendering with a type-safe mapping betweenCardTypeenum and React components - Introduced
BaseRecordLayoutconstant for configurable tab definitions with granular visibility rules based on context, features, and dependencies - Added new types (
RecordLayoutTab,TabVisibilityConfig,LayoutCard) to support structured tab configuration and visibility control - Refactored
useRecordShowContainerTabsto use position-based ordering and evaluate visibility based on metadata, features, and relations - Modified
ShowPageSubContainerto use the new card component mapping system, replacing the previous switch statement implementation
11 file(s) reviewed, 11 comment(s)
Edit PR Review Bot Settings | Greptile
| const StyledGreyBox = styled.div<{ isInRightDrawer?: boolean }>` | ||
| background: ${({ theme, isInRightDrawer }) => | ||
| isInRightDrawer ? theme.background.secondary : ''}; | ||
| border: ${({ isInRightDrawer, theme }) => | ||
| isInRightDrawer ? `1px solid ${theme.border.color.medium}` : ''}; | ||
| border-radius: ${({ isInRightDrawer, theme }) => | ||
| isInRightDrawer ? theme.border.radius.md : ''}; | ||
| height: ${({ isInRightDrawer }) => (isInRightDrawer ? 'auto' : '100%')}; | ||
|
|
||
| margin: ${({ isInRightDrawer, theme }) => | ||
| isInRightDrawer ? theme.spacing(4) : ''}; | ||
| `; |
There was a problem hiding this comment.
style: StyledGreyBox uses empty strings as fallback values which could lead to invalid CSS. Consider using 'transparent' for background and 'none' for border/border-radius instead of empty strings.
packages/twenty-front/src/modules/object-record/record-show/components/CardComponents.tsx
Show resolved
Hide resolved
packages/twenty-front/src/modules/object-record/record-show/constants/BaseRecordLayout.ts
Show resolved
Hide resolved
| position: 0, | ||
| Icon: IconPrinter, |
There was a problem hiding this comment.
logic: Multiple workflow components have position: 0 which may cause unpredictable tab ordering
packages/twenty-front/src/modules/object-record/record-show/hooks/useRecordShowContainerTabs.ts
Show resolved
Hide resolved
packages/twenty-front/src/modules/ui/layout/tab/types/RecordLayoutTab.ts
Show resolved
Hide resolved
packages/twenty-front/src/modules/ui/layout/tab/types/RecordLayoutTab.ts
Show resolved
Hide resolved
packages/twenty-front/src/modules/ui/layout/tab/types/RecordLayoutTab.ts
Show resolved
Hide resolved
| ifMobile: boolean; | ||
| ifDesktop: boolean; | ||
| ifInRightDrawer: boolean; | ||
| ifFeaturesDisabled: FeatureFlagKey[]; |
There was a problem hiding this comment.
style: Consider renaming ifFeaturesDisabled to ifFeatureFlagsDisabled for clarity since it specifically refers to feature flags
|
Thanks @ehconitin for your contribution! |
@FelixMalfait WDYT?
We can refactor shouldDisplay Files/Tasks/Notes Tab etc into a hook.