-
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
[UsersProfilePopover] Fix email sometimes is not visible #184318
Conversation
<EuiHighlight search={searchValue}>{displayName}</EuiHighlight> | ||
</div> | ||
{option.user.email && option.user.email !== displayName ? ( | ||
<div className="eui-textTruncate" css={{ fontSize: '0.8em', lineHeight: 1.2 }}> |
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.
may we just use EuiText
component here? with color
and size
options
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.
agree, looks better 👍
The new changes LGTM for Cases! About the width, I am not sure to be honest. I think it depends if we want to be consistent across Kibana. I will leave the decision to @MichaelMarcialis or @mdefazio. |
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.
LGTM!
Pinging @elastic/appex-sharedux (Team:SharedUX) |
@elasticmachine merge upstream |
Yes, I think reducing the cases popover to a more reasonable width would be warranted with the improvements you've made. |
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.
Thanks for putting this together, @Dosant. It looks great. I've left a few small comments below, but nothing worth holding you up for. Assuming you're able to address them, I'll approve now.
My first comment is regarding the assignee dropdown menu when creating a new case. It looks like this new stacking of display name and email wasn't applied here. Can/should we do so?
<div> | ||
<strong>{displayName}</strong> | ||
</div> | ||
{user.email && user.email !== displayName ? <div>{user.email}</div> : undefined} |
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.
Should we take the same approach in the tooltips that we are for the selectable lists? For example, should we:
- Truncate both the display name and email to a single line.
- Remove the
strong
element around the display name. - Reduce the email font size to 12px.
Doing so may be more consistent and yield a less crowded tooltip in situations with lengthy names/emails. 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.
Truncate both the display name and email to a single line.
I initially did this but then reverted. I concluded that the idea of a tooltip is to show the full information, even if it is not that nice visually. So decided to better wrap, not truncate.
Remove the strong element around the display name.
Reduce the email font size to 12px.
agree, changed 👍
<EuiText | ||
size={'xs'} | ||
color={option.disabled ? 'disabled' : 'subdued'} | ||
className="eui-textTruncate" | ||
> | ||
{searchValue ? ( | ||
<EuiHighlight search={searchValue}>{option.user.email}</EuiHighlight> | ||
) : ( | ||
option.user.email | ||
)} | ||
</EuiText> |
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.
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.
Just checked that it isn't flexible, this would require a EUI change.
Text decoration is applied on the whole euiSelectableListItem__text and the text decoration color is not changed if I change it on a child . euiText with email
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.
@MichaelMarcialis, thanks for the review, I improved what I could
My first comment is regarding the assignee dropdown menu when creating a new case. It looks like this new stacking of display name and email wasn't applied here. Can/should we do so?
Looks like this is a different component :(
I won't touch it
Also in that case because how wide the input is, it should fit the longer names nicely. Unfortunetly, not consistent with this other component
<EuiText | ||
size={'xs'} | ||
color={option.disabled ? 'disabled' : 'subdued'} | ||
className="eui-textTruncate" | ||
> | ||
{searchValue ? ( | ||
<EuiHighlight search={searchValue}>{option.user.email}</EuiHighlight> | ||
) : ( | ||
option.user.email | ||
)} | ||
</EuiText> |
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.
Just checked that it isn't flexible, this would require a EUI change.
Text decoration is applied on the whole euiSelectableListItem__text and the text decoration color is not changed if I change it on a child . euiText with email
<div> | ||
<strong>{displayName}</strong> | ||
</div> | ||
{user.email && user.email !== displayName ? <div>{user.email}</div> : undefined} |
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.
Truncate both the display name and email to a single line.
I initially did this but then reverted. I concluded that the idea of a tooltip is to show the full information, even if it is not that nice visually. So decided to better wrap, not truncate.
Remove the strong element around the display name.
Reduce the email font size to 12px.
agree, changed 👍
💚 Build Succeeded
Metrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: cc @Dosant |
Summary
fix #182945
UsersProfilePopover
to always show email on the 2nd line. To keep the list virtualized had to increase the height a bit for every rowBefore:
After:
<UserAvatarTip/>
doesn't handle long names well #182561 and adds email to Avatar "name" so it appears in a native tooltipThe only concern that I have is that cases popover was working around that issue with email visibility by making the popover very wide, I wonder if this fix is OK for them and if we could reduce the width now? cc @cnasikas @MichaelMarcialis
The downside of this fix is that each row takes a bit more height.