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

Fix: Make documentation button hit area the whole button in Shell box #809

Open
wants to merge 2 commits into
base: master
from

Conversation

@JohnPhamous
Copy link

JohnPhamous commented Aug 10, 2020

Description

No visual changes were made. The "Read documentation" button was a button with an a inside. This reduces the hit area of the button that results in an action (opening the documentation) only the text bounds.

The fix is to apply the button styles directly to the a. This PR also makes keyboard/AT navigation a lot better, an accessibility win!

This PR also updates the button's styles to match the mockup in Figma.

Related Issues

@JohnPhamous
Copy link
Author

JohnPhamous commented Aug 10, 2020

/preview

@github-actions
Copy link

github-actions bot commented Aug 10, 2020

Please find a preview at: https://staging.nodejs.dev/809/

John Pham
@designMoreWeb
Copy link

designMoreWeb commented Aug 10, 2020

Thanks for the contribution this looks good to me

@bnb
Copy link
Member

bnb commented Aug 11, 2020

Semantically, wouldn't a button be More Correct HTML? Definitely not opposed to making the button area bigger, but ideally we'd be using semantically correct HTML ❤️

@JohnPhamous
Copy link
Author

JohnPhamous commented Aug 11, 2020

Semantically, wouldn't a button be More Correct HTML? Definitely not opposed to making the button area bigger, but ideally we'd be using semantically correct HTML ❤️

I think a button makes more sense if it was performing an action on the page or navigating internally. Since this opens an external link, it should be treated as a link. I'm using this as a reference.

As a sanity check, I checked the landmarks/navigation with a screen reader and it sounds alright.

@bnb
Copy link
Member

bnb commented Aug 11, 2020

@JohnPhamous fair enough. fwiw eventually this should open the internal link to a docs implementation rather than the external existing docs site.

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

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.