-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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 UsersProfilePopover
doesn't handle long names well
#182155
Conversation
@@ -340,12 +342,24 @@ export const UserProfilesSelectable = <Option extends UserProfileWithAvatar | nu | |||
gutterSize="s" | |||
responsive={false} | |||
> | |||
<EuiFlexItem> | |||
<EuiHighlight search={searchValue}>{option.label}</EuiHighlight> | |||
<EuiFlexItem style={{ minWidth: 0, flexBasis: 'auto' }}> |
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.
@elastic/eui-team could you take a look please, I've tried a combination of different options and this is what I've ended up with. Not sure if this is the best way from UI and code perspective. Would appreciate any feedback.
One possibility is to also not show an email in case the user label is too long, but, as far as I understand, this is non-trivial as we don't know ahead of rendering if the label can fit.
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.
From a code perspective, the flex CSS looks reasonable to me! Just curious, is there any reason to use an inline style
here over css={{ minWidth: 0, flexBasis: 'auto' }}
?
One possibility is to also not show an email in case the user label is too long, but, as far as I understand, this is non-trivial as we don't know ahead of rendering if the label can fit.
You could possibly "work around" that in CSS by letting the user label grow to 1 and the email shrink to 0 width with overflow hidden 🤔 It's up to you how important you think that is though. We might want to loop in a designer like @MichaelMarcialis for input on the UI portion of things.
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.
Thank you!
ust curious, is there any reason to use an inline style here over css={{ minWidth: 0, flexBasis: 'auto' }}
😅 I still don't have much experience with emotion and don't know many details about how it works under the hood. Intuitively I am afraid that it adds some overhead, so I am inclined to switch to inline styles when there is no need for emotion. Just curious to learn more:
- Is there any advantage in using emotion here (apart from consistency)?
- regarding the other comment with DRY Fix
UsersProfilePopover
doesn't handle long names well #182155 (comment), is there also a performance advantage in moving the styles up?
You could possibly "work around" that in CSS by letting the user label grow to 1 and the email shrink to 0 width with overflow hidden
I gave it a try by adding flexShrink:0 to the name (maybe I misunderstood). Couldn't make it work, because in this case name truncation doesn't work. I am probably missing something
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.
I gave it a try by adding flexShrink:0 to the name (maybe I misunderstood). Couldn't make it work, because in this case name truncation doesn't work. I am probably missing something
Try this on line 343 instead:
<EuiFlexItem css={{ maxWidth: '100%' }}>
<EuiHighlight className="eui-textTruncate" search={searchValue}>
with just that small tweak you should be all set, the email will get completely hidden if the name is too long!
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.
Intuitively I am afraid that it adds some overhead, so I am inclined to switch to inline styles when there is no need for emotion.
From my understanding in production Emotion is quite performant compared to dev and perf impact is negligible. To clarify, we're definitely in the microperf territory where this kind of thing only starts to matter in the thousands of renders, so it likely doesn't matter either way and it's definitely not a blocking comment. FWIW I do believe that rendering thousands of inline styles is significantly less performant than thousands of classNames.
Is there any advantage in using emotion here (apart from consistency)?
Specificity would be the biggest one - inline styles have way higher specificity than CSS, and can be frustrating and annoying for other devs to override. Probably doesn't super matter in this scenario other than as a general best practice.
regarding the other comment with DRY #182155 (comment), is there also a performance advantage in moving the styles up?
Very minor but yes. Again we're talking microperf territory here, so please don't take this article as gospel especially for a component with very few renders/rerenders, but here's some more info: https://engineering.deptagency.com/optimizing-emotion-in-react
I would consider your current usage at this point more than fine, with no need to pull out the css
prop to a static const.
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.
Thank you! The trick works.
Thanks for taking the time explaining about emotion
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.
We might want to loop in a designer like @MichaelMarcialis for input on the UI portion of things.
Apologies for the late reply. Generally speaking, I think truncation to ellipses is a fine way to address a lack of horizontal space. However, it would be unfortunate to lose all mention of an accompanying email address, if that is indeed what is happening here (as that may be important in helping identify users).
Is this always the maximum width of this filter's popover? Perhaps we can increase this a bit to offer a bit more breathing room? Additionally, off the top of my head I'd propose one of the following:
-
Option 1: Move the optionally appearing email address to a line below the user name and align the text left (so it aligns nicely with the user name above it). In doing so, I'd also recommend reducing the font size to its extra small variant. This way, both pieces of text can have text truncation applied, without losing either entirely.
-
Option 2: If it is preferable to keep the user name and email address on the same line, apply a reasonable maximum width to the email address container, have the user name container fill the remaining available horizontal space and set both to truncate their text (so both are visible/represented, even if truncated heavily).
Thoughts?
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.
Option 1: Move the optionally appearing email address to a line below the user name and align the text left (so it aligns nicely with the user name above it). In doing so, I'd also recommend reducing the font size to its extra small variant. This way, both pieces of text can have text truncation applied, without losing either entirely.
Not a UX/UI person or anything, but I really like this option from a functional perspective :) It's surprising how many namesakes a large organization might have, and having visibility to email might be vital.
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.
Great feedback, let's create an issue (I'll create)
packages/kbn-user-profile-components/src/user_profiles_selectable.tsx
Outdated
Show resolved
Hide resolved
Pinging @elastic/appex-sharedux (Team:SharedUX) |
💚 Build Succeeded
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: cc @Dosant |
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.
Looks good, thank you for the fix!
Summary
Fix #182084
Before
After