-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Fix array and links display #8671
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 simplifies and standardizes the display of arrays and links across the application by removing unnecessary styling complexity and focus state handling.
- Replaced custom styled tags with Chip components in
packages/twenty-front/src/modules/ui/field/display/components/ArrayDisplay.tsx
- Removed focus state handling from both ArrayFieldDisplay and LinksFieldDisplay components
- Improved URL type checking regex patterns in
packages/twenty-front/src/utils/checkUrlType.ts
for better LinkedIn and Twitter URL handling - Standardized link display using ExpandableList with RoundedLink and SocialLink components
- Removed unused
isInputDisplay
prop from ArrayFieldMenuItem component
6 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings | Greptile
packages/twenty-front/src/modules/ui/field/display/components/ArrayDisplay.tsx
Outdated
Show resolved
Hide resolved
{value?.map((item) => ( | ||
<Chip variant={ChipVariant.Highlighted} label={item} /> | ||
))} |
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: Missing key prop for mapped elements
return isFocused ? ( | ||
<ExpandableList isChipCountDisplayed> | ||
return ( | ||
<ExpandableList> | ||
{links.map(({ url, label, type }, index) => |
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: Using array index as key may cause issues with React reconciliation if links are reordered
url, | ||
) | ||
) { | ||
if (/^(https?:\/\/)?(www\.)?linkedin\.com(\/[\w\-\/?#:.%=&]*)?$/.test(url)) { |
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: The new regex pattern allows invalid LinkedIn URLs that end with .linkedin.com (e.g. fake.linkedin.com)
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.
LGTM
Before
After