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

truncate the string if overlow the text #6166

Merged

Conversation

RobertoSimonini1
Copy link
Contributor

@RobertoSimonini1 RobertoSimonini1 commented Jul 8, 2024

This PR aims to fix #6102

I think we should dicuss a bit about how we should manage the lenght and set the variables globally

Edit :

@RobertoSimonini1 I used this PR to fix various problems that were left unsolved :

  • Refactor TextDisplay component, EllipsisDisplay was redundant with OverflowingTextWithTooltip
  • Removed maxWidth on TextDisplay for all other components, as it wasn't the right way to do it, the parent container should be responsible for width not the TextDisplay (code smell)
  • Drilled-down isCentered to respect its intent in the RecordInlineCell display of the record show page title
  • Fixed RecordInlineCellEditMode so that the portal is well centered above the record show page title
  • Fixed DoubleTextInput width so that it expands normally and takes all its parent available space.

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

  • Truncate full name in packages/twenty-front/src/modules/object-record/record-field/meta-types/display/components/FullNameFieldDisplay.tsx based on device type (15 chars for mobile, 20 for desktop)
  • Adjust max-width in packages/twenty-front/src/modules/ui/field/display/components/EllipsisDisplay.tsx to handle text overflow properly

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

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

(updates since last review)

  • No major changes found since last review.

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

@FelixMalfait
Copy link
Member

Hey Roberto, I think it should adapt to the parent container's size to avoid harcoding any value indeed. Also we want to leverage CSS' native ellipsis feature whenever we can instead of using Javascript. Can you try it this way? There are already examples of Ellipsis with a tooltip component in the codebase

@RobertoSimonini1
Copy link
Contributor Author

@FelixMalfait oh ok Felix, cool, I'll work on this as soon as possible, I think tomorrow I'll have some time :)

@lucasbordeau lucasbordeau self-requested a review July 19, 2024 09:24
@lucasbordeau lucasbordeau merged commit 7b615cf into twentyhq:main Jul 19, 2024
10 of 11 checks passed
Copy link

Thanks @RobertoSimonini1 for your contribution!
This marks your 1st PR on the repo. You're top 38% of all our contributors 🎉
See contributor page - Share on LinkedIn - Share on Twitter

Contributions

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.

Display issue with long record names
3 participants