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

Link component needs a11y compatibility updates #334

Open
kieckhafer opened this issue Oct 15, 2018 · 5 comments
Open

Link component needs a11y compatibility updates #334

kieckhafer opened this issue Oct 15, 2018 · 5 comments

Comments

@kieckhafer
Copy link
Member

Type: minor

Describe the bug
The Link component has two possible outputs: an a tag, which is used when href is present, and a div tag, which is used when href is not present.

The div version of the component needs to be updated for a11y compatibility. This can be done in one of two ways:

  1. Adding keyboard event listeners onto the div
  2. Creating an unstyled version of our Button component, and swapping the div for a button

Option 2 is probably the best long term solution. but this should be discussed before proceeding with any work this ticket.

If option 1 is selected, we should document all KeyBoard events that need to be added in order to reach a11y compliancy, and create a checklist for future use.

@aldeed
Copy link
Contributor

aldeed commented Oct 15, 2018

Button already has a "text button" style, and there is a version of that without the padding. We should have a discussion about how a non-href link is any different from that.

Also, I believe Link was primarily created as a placeholder and we expect people would swap in a router-specific Link component. But that pattern is a little fuzzy to me and probably not documented.

@kieckhafer
Copy link
Member Author

Even the text "no-padding" version has top and bottom padding. I guess it's named awkwardly, it's "no-horizontal padding". We could remove all padding and make it truly a text only no styling thing.

@aldeed
Copy link
Contributor

aldeed commented Oct 16, 2018

@rymorgan @cassytaylor I think we need some design input here to know what your intent is.

Then there's also a technical component, though, in that everything could be a text button if we didn't need SEO crawlability, but we do. But it's not always clear when we need that because any component could be dropped onto a secured page or a public page needing SEO. And in order to do links properly in NextJS, we replace the default Link component with the one provided by NextJS anyway. There must be some way to get everything we need and have simple rules about when to use what.

@cassytaylor
Copy link

@kieckhafer @aldeed I'm not 100% sure if I understand the problem, but if it's a matter of styling, would it be possible to just underline the link text to convey that it's a link?

Unrelated, but it seems like that (underlined text as a link style) should be added to our CL anyway, since text buttons with just top & bottom padding look kinda wonky

@aldeed
Copy link
Contributor

aldeed commented Oct 18, 2018

@cassytaylor Yes, I guess there are a couple questions here.

We should be careful about the terms "link" and "button". That should be a technical decision based on whether we need links that crawlers can follow, but you may choose to design based on that information.

In other words, something that looks like underlined text in the design might be best implemented as a button element, while something that looks like a button might be best implemented as a link element.

So I think what we need is a single "simple text with underline" text type, and we'll make both the Button component and the Link component capable of rendering that way. (Maybe the text-only, no-padding button can just be adjusted to look like this, removing the top/bottom padding and hover background.)

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

No branches or pull requests

3 participants