search

reactioncommerce / reaction-component-library

Example Storefront Component Library: A set of React components for the Example Storefront
Apache License 2.0
96 stars 57 forks source link

Link component needs a11y compatibility updates #334

Open kieckhafer opened 5 years ago

kieckhafer commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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