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

Anchor with Button styles #430

Open
hobadams opened this issue Aug 4, 2020 · 9 comments
Open

Anchor with Button styles #430

hobadams opened this issue Aug 4, 2020 · 9 comments
Labels
discussion PR or Issue requires a discussion.

Comments

@hobadams
Copy link

hobadams commented Aug 4, 2020

Is your feature request related to a problem? Please describe.
I would like to add an anchor tag that has the styles of the <Button> components.

Describe the solution, feature, or improvement you'd like
I think the easiest solution would be for the <Button> component to accept an as prop. We can then pass <Button as="a">.

In my opinion this would be a great option for all of the components. It would mean you could use <Box as="section"> etc.

Apologies if this is already possible, I couldn't see it in the docs.

Describe alternatives you've considered
An alternative is to have all the button styles available to css classes. This seems quite bloated though

Additional context
N/A

@chanceaclark
Copy link
Contributor

chanceaclark commented Aug 4, 2020

Hey @hobadams,

Thanks for submitting this issue!

Before I get into the weeds of the root issue, I'd like to point out that we do have the ability to use as on some of our components - they should be on the prop tables if you can use it. For example the Box component, but there are some components that we restrict to specific tags like our Text component. You'll just have to look at the code to see those restrictions.

Okay, so down to using Button as a Link – this breaks accessibility, and here's why....

Buttons are traditionally used to express actions, things that users interact with on the current page. While anchors are used for navigating to different pages. Take text context out of the equation, but when you use a link that looks like a button it tricks a user that they are going to interact with something on a page instead of navigating. The same is true about the inverse - treating an anchor like button.

Also, TypeScript cannot handle dynamic props like this. Say we use <Button as="a"> we assume we have all the anchor props, but it breaks our interface interface ButtonProps extends ButtonHTMLAttributes<HTMLButtonElement>. Notice how the props are HTML <button> props. TypeScript cannot determine what interface to use based on the runtime props.

IMO I am against doing this. It should be on the designers to use the appropriate components and on the developers to safe-guard this notion.

@hobadams
Copy link
Author

hobadams commented Aug 6, 2020

Hey @chanceaclark thanks for your response I appreciate the detailed feedback.

I still feel like it's quite a common requirement to have anchors that have the look of buttons. I agree with some of your points about accessibility but feel that if the content / title / aria-label of the anchor or button is descriptive enough then it's not a huge issue.

I also appreciate that dynamic props would cause an issue.

Perhaps my request makes more sense out of the context of a BigCommerce app.

What are your thoughts on adding some of the styles to css classes?

@chanceaclark
Copy link
Contributor

I still feel like it's quite a common requirement to have anchors that have the look of buttons. I agree with some of your points about accessibility but feel that if the content / title / aria-label of the anchor or button is descriptive enough then it's not a huge issue.

I have to respectfully disagree with this. I encourage you to read about the first and second rule of ARIA:

I also encourage you to read this article on why it's such a bad practice in the first place:

If you need a button to behave like a link, wrap a button in an anchor and disable the tabindex on the button. There are specific behaviors each of the elements outlined in the above article that is intended for each element.

<a href="#">
    <button tabindex="-1">Learn More</button>
</a>

Perhaps my request makes more sense out of the context of a BigCommerce app.

IMO this is more of a fundamental HTML/CSS/A11y issue more than in-or-out of the BigCommerce app context.

What are your thoughts on adding some of the styles to css classes?

BigDesign is not a CSS library, at least in this stage of the library. One of the main goals of the library is to allow apps and app developers make their apps and integrations look and feel like it was apart of the BigCommerce ecosystem. That being said, we have tailored this React library to lock stylings so that developers don't stray from uniform design patterns. As of this stage of the library, we are against adding any CSS classes until we have further discussions around if/how to support it.

@chanceaclark chanceaclark added the discussion PR or Issue requires a discussion. label Apr 30, 2021
@leeBigCommerce
Copy link
Contributor

leeBigCommerce commented May 27, 2021

I'm not sure we should wrap buttons inside as as it's invalid HTML:

https://html.spec.whatwg.org/multipage/text-level-semantics.html#the-a-element

there must be no interactive content descendant

@hobadams
Copy link
Author

I'm not sure we should wrap buttons inside as as it's invalid HTML:

https://html.spec.whatwg.org/multipage/text-level-semantics.html#the-a-element

there must be no interactive content descendant

Thanks @leeBigCommerce . That wasn't the intention. It was to allow anchor tags to inherit the core button styles.

The use case would be if you wanted to have a big button that linked to your docs or some other url. This SHOULD be an anchor but you may want it to look like a button.

@hobadams
Copy link
Author

I'm not sure we should wrap buttons inside as as it's invalid HTML:

https://html.spec.whatwg.org/multipage/text-level-semantics.html#the-a-element

there must be no interactive content descendant

@leeBigCommerce apologies, just saw the comment above and now know why you said this! I agree with you, a button inside an anchor is a no go.

@chanceaclark
Copy link
Contributor

Been thinking about this for a bit now... I'm starting to see the other side of this argument. I'm still debating in my mind the user-experience aspect of it, but for the DX part of it should be fairly simple to implement (to put it simply, a button with an href attribute will render a link).

@beppek
Copy link

beppek commented Dec 31, 2021

@chanceaclark it's a fairly common design pattern, whether it's correct usage or not, and I'd say people are pretty used to it by now. I doubt you'll confuse anyone as long as the HTML is semantically correct and proper aria labels are applied.

If you only enable it by adding a href attribute, it doesn't allow using the built-in navigation of React-based frameworks like Gatsby, Next and Remix. It would break data prefetching and force the entire app interface to reload, clearing things such as global state unless it's cached in the browser's built-in storage.

For example, other component libraries, such as Chakra UI, allow you to pass an as prop to Buttons, so they render as Links. It's also not restricted to Chakra components. For example, it can be used for Next/Gatsby/Remix's Link component, passing on any additional props (such as href for Next or to for Gatsby/Remix) to the rendering component.

I'm not a TypeScript ninja, and I never use strict, so I can't say its implications on TS. I don't get IntelliSense that href exists as a prop if I pass as={Link}, but it also doesn't give me an error when I add it. I could swear I've had linter errors in the past if I didn't pass href, but I don't get them now, so I might have imagined it. It breaks in the build stage, though, as you'd expect.

@chanceaclark
Copy link
Contributor

Hey all, I threw up a draft PR regarding this #663. Still having some type issues but I'll get back to this in the new year.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion PR or Issue requires a discussion.
4 participants