-
Notifications
You must be signed in to change notification settings - Fork 436
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 long participant name + status in participant list with tooltip #4548
Conversation
@skjnldsv FYI in case you want to check it out |
handleResize() { | ||
const e = this.$refs.userName | ||
this.showUserNameTooltip = (e && e.offsetWidth < e.scrollWidth) | ||
const e2 = this.$refs.userStatus | ||
this.showUserStatusTooltip = (e2 && e2.offsetWidth < e2.scrollWidth) | ||
}, |
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.
Is this really needed? I feel like a simple would suffice.
It's fine if we just show it for all sizes. It would avoid the cost of a ResizeObserver as well as increase accessibility :)
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.
it looks shitty with short names...
now, there is another possible, reusable but more complex solution: extend the Tooltip directive to be able to check an element's ellipsization only on hover and then decide whether to display or not. Or at least provide some events so that the caller could do the check on their own.
I'm fine removing all the conditionals here if we are fine with the above look...
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.
Well titles appear on long-hover only, I think it's fine.
I'm afraid a resizeobserver for each participant just wouldn't scale.
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.
ok, I've removed the conditions
it's less bad than I think as the hover will only appear if you hove on the text itself, not the whole row, so it's ok
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.
Couldn't we just observe the resize one level above? we only need to do it once for all participant components. I think that the bob tooltip looks really bad.
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.
Even if we observe in the ParticipantList, we still need to trigger an event to all the participant entries individually so they can perform the tooltip check.
The best solution is to have an on-demand check, basically whenever the user hovers, the check is done, and then the decision is made whether the tooltip appears or not. Ideal would be to have this as an option in the v-tooltip directive.
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've raised nextcloud-libraries/nextcloud-vue#1567 to address this there.
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.
if this is the lib we use there is a chance: https://github.com/Akryum/v-tooltip
I see there are event handlers, I'll see if I can prevent displaying...
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 checked the library and it seems we cannot use it that way.
See my attempts here: nextcloud-libraries/nextcloud-vue#1567 (comment)
Now I found another trick that works without touching the lib: within a "mouseover" handler, do the ellipsis check and quickly update the tooltip text afterwards. That seems to work for me. See 40d87af
87dbbe4
to
01049a5
Compare
/backport to stable20 |
Signed-off-by: Vincent Petry <vincent@nextcloud.com>
For when participant names or status texts are too long, the user can hover on them to view them in a tooltip. Signed-off-by: Vincent Petry <vincent@nextcloud.com>
When the mouse hovers on a participant name or status, detect whether that one is ellipsized. If yes, set the matching tooltip text to display it. Otherwise, let the text empty so the tooltip doesn't appear. Signed-off-by: Vincent Petry <vincent@nextcloud.com>
01049a5
to
40d87af
Compare
Description
Added ellipsis on long participant names.
Added conditional tooltip on participant + status:
Whenever the participant name or status is ellipsized, a tooltip will be available for those entries.
To check the "is ellipsized" condition, a ResizeObserver is used to update. To avoid trying to measure and update too many participants in the list, the observer only activates after 10+ characters, which is the limit of chinese characters that still fit within the sidebar minimum width of 300px.
Note that in the future once we use virtual scroll for the participant list, we likely won't need to worry about resize optimization.
Fixes #4500
Tests
Long status ellipsis + tooltip
Long user name + tooltip
Note: if the user name has no space, the tooltip breaks the layout, this is a bug in the tooltip library and needs to be addressed there.