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

[Fix] - Trim Names in Settings > Members table #7509 #7525

Merged

Conversation

Karankhatik
Copy link
Contributor

@Karankhatik Karankhatik commented Oct 9, 2024

Issue: Long names in the Members table were overflowing, affecting the layout.

Fix:

  • Trimmed long names with ellipses.
  • Added tooltips to display the full content on hover.
  • Max-width of the text dynamically set to 90px on large screens, and 60px on mobile.
    image

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 implements text trimming for long names in the Settings > Members table, improving layout and readability.

  • Added StyledScrollableTextContainer with max-width and ellipsis for name display
  • Implemented AppTooltip to show full names on hover
  • Adjusted StyledTableCell for better mobile responsiveness
  • Modified TableRow rendering to include new styling and tooltip functionality
  • Updated imports to include necessary components from 'twenty-ui'

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

@Bonapara
Copy link
Member

Bonapara commented Oct 9, 2024

Current:

CleanShot 2024-10-09 at 14 32 20

It looks like there is an undesired right padding. The gap between the name and the email should be only 16px

Desired:

CleanShot 2024-10-09 at 14 36 26

@charlesBochet
Copy link
Member

Hi @Karankhatik, thanks for the PR
The code looks good, waiting on @Bonapara feedbacks to be taken into account

@Karankhatik
Copy link
Contributor Author

Hello @charlesBochet what should I do, should I work on @Bonapara feedback or code is fine. Please confirm.

@charlesBochet
Copy link
Member

Hello @charlesBochet what should I do, should I work on @Bonapara feedback or code is fine. Please confirm.

Work on @Bonapara feedbacks :)

@Karankhatik
Copy link
Contributor Author

Karankhatik commented Oct 10, 2024

@Bonapara I've successfully removed the undesired space; however, there's an issue where, if the name is shorter then the (ellipsis length), the email gets too close to the name, which affects the layout. Please provide your suggestions.
image
image

@Bonapara
Copy link
Member

Bonapara commented Oct 10, 2024

@Karankhatik The issue isn't the spacing between the fields, which should remain the same. It's the name field that isn't using its full available width.

The spacing between the fields should remain 16px

@charlesBochet
Copy link
Member

You also have conflicts now :)

@Karankhatik
Copy link
Contributor Author

@Bonapara, you mean the text should take up the entire width of the container, and only when it exceeds the container's width, we apply an ellipsis? Also, the space between the name and email will be 16px. Is that correct?

@Bonapara
Copy link
Member

Absolutely!

@Karankhatik
Copy link
Contributor Author

Karankhatik commented Oct 11, 2024

Thanks will update u.

karankhatik added 2 commits October 12, 2024 15:31
@@ -97,6 +103,26 @@ const StyledTableHeaderRow = styled(Table)`
margin-bottom: ${({ theme }) => theme.spacing(1.5)};
`;

const StyledTableRowForMemberList = styled(TableRow)`
gap: 16px;
@media (max-width: 768px) {
Copy link
Member

Choose a reason for hiding this comment

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

all these cases look too heavy, I'm taking a look

@charlesBochet
Copy link
Member

Mobile:
image

@charlesBochet
Copy link
Member

Desktop:
image

Copy link
Member

@charlesBochet charlesBochet left a comment

Choose a reason for hiding this comment

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

I've reworked a bit to simplify CSS

@charlesBochet charlesBochet merged commit 1e6346f into twentyhq:main Oct 13, 2024
11 checks passed
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.

3 participants