-
Notifications
You must be signed in to change notification settings - Fork 95
feat(NcUserBubble): add RouterLink support #5708
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
Conversation
ShGKme
left a comment
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.
This PR is for: nextcloud/spreed#12510
There NcUserBubble is used as a clickable div to implement manual router navigation. It is a UX and a11y issue (how to open it in a new tab or via keyboard?).
Instead, it's better to add RouterLink support to this component. Then it will be actually an interactive link element.
Without additional context (spreed PR), clickable div itself should be already an issue. Maybe we should extend it to |
Component already supports With Popover it is still clickable |
07f5409 to
4eb1820
Compare
Signed-off-by: Maksim Sukharev <[email protected]>
4eb1820 to
6bae996
Compare
| return this.hasUrl ? 'a' : 'div' | ||
| return this.hasUrl | ||
| ? (this.to ? RouterLink : 'a') | ||
| : 'div' |
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.
Resolved your suggestions, thanks!
I kept only RouterLink support, and cursor styling for anchors.
Can we address this in follow-up, maybe? or <a role="button"> would suffice?
| : 'div' | |
| : (this.popoverEmpty ? 'div' : 'button') |
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.
Yes, let's do it in a follow up. It is not a part of the original issue. TBH I even don't remember, where we use use bubble with popover...
DorraJaouad
left a comment
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
|
/backport to next |
☑️ Resolves
pointerif an element has click event🖼️ Screenshots
🏁 Checklist
nextrequested with a Vue 3 upgrade