Skip to content
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

feat(NcUserBubble): add RouterLink support #5708

Merged
merged 1 commit into from
Aug 5, 2024

Conversation

Antreesy
Copy link
Contributor

@Antreesy Antreesy commented Jun 14, 2024

☑️ Resolves

  • Show interactive cursor pointer if an element has click event

🖼️ Screenshots

🏚️ Before 🏡 After
image image

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable
  • 3️⃣ Backport to next requested with a Vue 3 upgrade

@Antreesy Antreesy added 3. to review Waiting for reviews design Design, UX, interface and interaction design feature: userbubble Related to the userbubble component labels Jun 14, 2024
@Antreesy Antreesy added this to the 8.12.1 milestone Jun 14, 2024
@Antreesy Antreesy self-assigned this Jun 14, 2024
Copy link
Contributor

@ShGKme ShGKme left a 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.

@ShGKme ShGKme added bug Something isn't working and removed design Design, UX, interface and interaction design labels Jun 14, 2024
@Antreesy
Copy link
Contributor Author

here NcUserBubble is used as a clickable div

Without additional context (spreed PR), clickable div itself should be already an issue. Maybe we should extend it to <a>/<button>/<div> at least?

@ShGKme
Copy link
Contributor

ShGKme commented Jun 14, 2024

here NcUserBubble is used as a clickable div

Without additional context (spreed PR), clickable div itself should be already an issue. Maybe we should extend it to <a>/<button>/<div> at least?

Component already supports <a> with href prop. We can also add to prop to support <RouterLink>.

With Popover it is still clickable <div>, but let's call it a separate issue

@susnux susnux modified the milestones: 8.12.1, 8.13.1, 8.14.0 Jun 25, 2024
@susnux susnux modified the milestones: 8.14.0, 8.15.0 Jul 4, 2024
@Antreesy Antreesy modified the milestones: 8.15.0, 8.15.1 Jul 23, 2024
@susnux susnux modified the milestones: 8.15.1, 8.15.2 Jul 29, 2024
@Antreesy Antreesy force-pushed the fix/noid/user-bubble-clickable branch from 07f5409 to 4eb1820 Compare August 3, 2024 08:24
@Antreesy Antreesy changed the title fix(NcUserBubble): indicate that element is clickable fix(NcUserBubble): add RouterLink support Aug 3, 2024
@Antreesy Antreesy requested a review from ShGKme August 3, 2024 08:25
@ShGKme ShGKme modified the milestones: 8.15.2, 8.16.0 Aug 3, 2024
Signed-off-by: Maksim Sukharev <antreesy.web@gmail.com>
@Antreesy Antreesy force-pushed the fix/noid/user-bubble-clickable branch from 4eb1820 to 6bae996 Compare August 3, 2024 13:51
return this.hasUrl ? 'a' : 'div'
return this.hasUrl
? (this.to ? RouterLink : 'a')
: 'div'
Copy link
Contributor Author

@Antreesy Antreesy Aug 3, 2024

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?

Suggested change
: 'div'
: (this.popoverEmpty ? 'div' : 'button')

Copy link
Contributor

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...

@ShGKme ShGKme added enhancement New feature or request and removed bug Something isn't working labels Aug 4, 2024
@ShGKme ShGKme changed the title fix(NcUserBubble): add RouterLink support feat(NcUserBubble): add RouterLink support Aug 4, 2024
Copy link
Contributor

@DorraJaouad DorraJaouad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Antreesy
Copy link
Contributor Author

Antreesy commented Aug 5, 2024

/backport to next

@Antreesy Antreesy merged commit 536f64f into master Aug 5, 2024
19 checks passed
@Antreesy Antreesy deleted the fix/noid/user-bubble-clickable branch August 5, 2024 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement New feature or request feature: userbubble Related to the userbubble component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants