[BUGFIX] Account owner should not be clickable & [Refactor] Chip.tsx links
#10359
Conversation
prastoin
force-pushed
the
prastoin-bugfix-10196
branch
from
65d77a2 to
89292dd
Compare
TODOs/FIXMEs:
|
prastoin
changed the title
Chip.tsx
prastoin
changed the title
Chip.tsxChip.tsx links
|
@greptileai trigger |
![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 chip components to enforce consistent link behavior and fixes the Account Owner field clickability issue, introducing a clearer separation between linked and non-linked chips.
- Introduced new
LinkChipandLinkAvatarChipcomponents in/packages/twenty-ui/src/display/chip/components/to handle link-specific functionality - Added
forceDisableClickprop in/packages/twenty-front/src/modules/object-record/components/RecordChip.tsxto prevent workspace member fields from being clickable - Extracted avatar chip logic into new
useGetAvatarChipLeftComponentAndVarianthook in/packages/twenty-ui/src/display/chip/hooks/ - Added
isModifiedEventutility in/packages/twenty-ui/src/utilities/events/for standardized handling of modified click events - Removed duplicate code by deleting
RecordIndexRecordChip.tsxand consolidating functionality intoRecordChip
19 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile
| @@ -145,7 +149,7 @@ export const AttachmentRow = ({ | |||
|
|
|||
| const handleOpenDocument = (e: React.MouseEvent) => { | |||
| // Cmd/Ctrl+click opens new tab, right click opens context menu | |||
| if (e.metaKey || e.ctrlKey || e.button === 2) { | |||
| if (isModifiedEvent(e) || e.button === 2) { | |||
There was a problem hiding this comment.
style: Consider adding e.button === 1 check for middle mouse button clicks which also typically open in new tab
| ? getLinkToShowPage(objectNameSingular, record) | ||
| : undefined | ||
| } | ||
| to={to ?? getLinkToShowPage(objectNameSingular, record)} |
There was a problem hiding this comment.
logic: getLinkToShowPage could return undefined, which would make the to prop undefined. According to the PR description, LinkChip must always have a defined to prop.
There was a problem hiding this comment.
If it's true you're a veryyy gooood bot, if not I still love you
| return ( | ||
| <StyledLink to={to} onClick={onClick}> | ||
| <Chip |
There was a problem hiding this comment.
logic: The disabled prop is not being passed to StyledLink, which means disabled chips will still be clickable links
| return ( | |
| <StyledLink to={to} onClick={onClick}> | |
| <Chip | |
| return disabled ? ( | |
| <Chip |
Introduction
Initially fixing the
Account Ownerrecord field value should not be clickable and redirects on current page bug.This has been fixed computing whereas the current filed is a workspace member dynamically rendering a stale Chip components instead of an interactive one
Refactor
Refactored the
AvatarChiptoprops logic to be scoped to lower level scopeChip.Now we have
LinkChipChip,LinkAvatarChipandAvatarChipall exported from twenty-ui.The caller has to determine which one to call from the design system
New rule regarding chip links
As discussed with @charlesBochet and @FelixMalfait
A chip link will now always have
todefined. ( and optionally anonClick).ChipLinkscannot be used as buttons anymoreFactorization
Deleted the
RecordIndexRecordChip.tsxfile ( akaRecordIdentifierChipcomponent ) that was duplicating some logic, refactored theRecordChipin order to handle what was covered byRecordIdentifierChipConclusion
As always any suggestions are more than welcomed ! Took few opinionated decision/refactor regarding nested long ternaries rendering
ReactNodeelementsMisc
Screen.Recording.2025-02-24.at.19.18.22.mov