Skip to content
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

Show email in RecordChip on Email Thread Right Drawer #6943

Closed
wants to merge 2 commits into from

Conversation

gvkhna
Copy link
Contributor

@gvkhna gvkhna commented Sep 8, 2024

Here's a simple PR to add the email to the record chip in the email threads view. This is really just an attempt to get my feet wet with the code base so I don't expect it to be accepted as-is. But wanted to get it looked at.

I understand there may be considerations about how to display the email. I would prefer a more muted color for the email text, perhaps a bit smaller, similar to how gmail does it. Also this is a simple way to get the email drilled through the many layers not sure if this is the desired method of extending the AvatarChip.

Here's what it looks like, this is a rather simple it just works type of commit.

Screenshot 2024-09-08 at 10 54 08

Also note, the Chip is currently not extending the maxWidth property which I had to add to get the email to not be truncated. Seems to work fine otherwise.

Update: apologies closed the previous, copied here, sorry for the mess, moved to it's own branch, opened as draft.

@gvkhna gvkhna marked this pull request as ready for review September 10, 2024 16:13
Copy link
Contributor

@greptile-apps greptile-apps bot left a 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 adds email display functionality to the RecordChip component in the email threads view, modifying several components to accommodate this new feature.

  • Added 'email' prop to ParticipantChip and RecordChip components in /packages/twenty-front/src/modules/activities/components/ParticipantChip.tsx and /packages/twenty-front/src/modules/object-record/components/RecordChip.tsx
  • Modified AvatarChip in /packages/twenty-ui/src/display/chip/components/AvatarChip.tsx to include email display and increase maxWidth when email is present
  • Introduced 'maxWidth' prop to Chip component in /packages/twenty-ui/src/display/chip/components/Chip.tsx for better width control
  • Potential UI layout and styling implications across the application due to changes in chip components

4 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings

Comment on lines +55 to +56
maxWidth={email ? 350 : 200}
label={email ? `${name} <${email}>` : name}
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 using a constant for maxWidth values instead of hardcoding

@@ -50,7 +52,8 @@ export const AvatarChip = ({

return (
<Chip
label={name}
maxWidth={email ? 350 : 200}
label={email ? `${name} <${email}>` : name}
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Email display format might need refinement for better UI/UX

@bosiraphael
Copy link
Contributor

Hello @gvkhna, thank you for contributing. Was there an issue created about this? Because I can't find this in the Figma design.
@Bonapara

@Bonapara
Copy link
Member

Indeed, we shouldn't put the person's name on the chip. Instead, we should display a tooltip when someone clicks on the to text, similar to how Gmail does it. We should add a chevron down to the right of the to text to indicate it's a dropdown.

CleanShot.2024-09-24.at.18.20.11.mp4

Figma

https://www.figma.com/design/xt8O9mFeLl46C5InWwoMrN/Twenty?node-id=40644-109692&node-type=instance&t=8dUCvXCxTOcmmFb0-11

Use the prototype to see the behavior by clicking play. tell me if you want to handle it. Otherwise happy to open a new issue!

Thanks @gvkhna

@bosiraphael
Copy link
Contributor

Closing this, you can create an issue @Bonapara :)

@Bonapara
Copy link
Member

#7264

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants