fix: removed scrollbar, elipsified overflow text supporting tooltip#17078
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR aims to improve the user experience by removing scrollbars and handling text overflow with ellipsis in the SettingsMorphRelationMultiSelect component dropdown items.
Changes:
- Added a new styled wrapper component (
StyledMenuItemWrapper) with flex layout and ellipsis overflow handling - Wrapped
MenuItemMultiSelectinstances within the new styled component to apply text truncation styling
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| > div > div { | ||
| overflow: hidden; | ||
| text-overflow: ellipsis; | ||
| white-space: nowrap; | ||
| } |
There was a problem hiding this comment.
The PR title mentions "supporting tooltip", but the implementation doesn't actually add tooltip functionality. The MenuItemMultiSelect component uses MenuItemLeftContent internally, which already has OverflowingTextWithTooltip support when text is a string. The wrapper div with ellipsis styles may interfere with the existing tooltip mechanism. Consider verifying that the tooltip still functions correctly, or if tooltip support needs to be explicitly added to this component.
| > div > div { | |
| overflow: hidden; | |
| text-overflow: ellipsis; | |
| white-space: nowrap; | |
| } |
|
|
||
| > div > div { | ||
| overflow: hidden; | ||
| text-overflow: ellipsis; | ||
| white-space: nowrap; | ||
| } |
There was a problem hiding this comment.
The CSS selector '> div > div' is too generic and fragile. It assumes a specific nested structure that may break if the internal structure of MenuItemMultiSelect changes. Since MenuItemMultiSelect internally uses MenuItemLeftContent which already has OverflowingTextWithTooltip (with built-in ellipsis handling), this styling may be redundant or conflicting. Consider either removing this wrapper entirely or using a more specific class-based selector if custom styling is truly necessary.
| > div > div { | |
| overflow: hidden; | |
| text-overflow: ellipsis; | |
| white-space: nowrap; | |
| } |
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:18106 This environment will automatically shut down when the PR is closed or after 5 hours. |
carbonFibreCode
changed the title
| gap: ${({ theme }) => theme.spacing(2)}; | ||
| min-width: 0; | ||
|
|
||
| > div > div { |
There was a problem hiding this comment.
let's avoid doing it. Is this the way it's done in other dropdowns?
I feel we should be using Existing TextWithOverflowingTooltip component. If not possible, we should add this style to the container itself and not using > div > div that will break when we change the DOM structure
charlesBochet
added
-PR: awaiting author
-PR: awaiting review
and removed
-PR: awaiting author
labels
|
I have made the fix, ty! |
|
Hey @charlesBochet! After you've done the QA of your Pull Request, you can mark it as done here. Thank you! |
1 similar comment
|
Hey @charlesBochet! After you've done the QA of your Pull Request, you can mark it as done here. Thank you! |
hey, thanks for the fix, i was trying to push changes rn but it's already merged... lol |
fixes #15152
Added CSS to ellipsify overflowing text with tooltip support, consistent with existing dropdown implementations such as the address dropdown for long country names.
This approach resolves the issue locally without modifying global components, avoiding the limitations of previous attempts.
Before :
After :