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

Userpicker view: Change anchor element to button #8738

Merged
merged 2 commits into from
Aug 31, 2020
Merged

Userpicker view: Change anchor element to button #8738

merged 2 commits into from
Aug 31, 2020

Conversation

BatJan
Copy link
Contributor

@BatJan BatJan commented Aug 26, 2020

Prerequisites

  • I have added steps to test this contribution in the description below

Description

This is a very very small PR changing the <a> tag to a <button> since it's not a link pointing to anywhere - Improves accessibility and screen reader announcement.

@abjerner
Copy link
Contributor

Hi @BatJan - I hope your doing well 😉

Changing the <a> to a <button> indeed makes sense, and is also in line with some of your past PRs, and it looks absolutely correct when skimming through the changes. Nevertheless, I'll try to give it a spin as well to be sure, and report back here with my findings.

Thanks 👍

@BatJan
Copy link
Contributor Author

BatJan commented Aug 27, 2020

Hey @abjerner Thank you mate 😃 I'm doing good thanks - Hope you're are well too 😎

@abjerner
Copy link
Contributor

Thanks @BatJan. Although a but to the busy side, I'm doing good as well 😄

The <button> element appears to be missing some styling to look like the <a> element:

  1. The <button> element doesn't take full width, so you can't toggle the user to by clicking the empty area to the left
  2. There is a bit of extra space to the left of the user name

image

In the screenshot above, the first item is with the <button> element, while the second item is with the <a> element (both shown with hover).

If you can fix this, it should be good to merge 😉

Copy link
Contributor

@abjerner abjerner left a comment

Choose a reason for hiding this comment

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

Aaaand I should have posted by comment as a review instead of a regular comment. If you can make the changes that I described in my previous comment, I'd be happy to merge 😉

@BatJan
Copy link
Contributor Author

BatJan commented Aug 31, 2020

@abjerner Good catch mate 👍 I've updated the styling it looks and acts like before the conversion to <button> 😎

@nul800sebastiaan nul800sebastiaan merged commit ae7290f into umbraco:v8/contrib Aug 31, 2020
@nul800sebastiaan
Copy link
Member

Looking good now indeed! 👍 Thanks @BatJan and @abjerner!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants