Fix disabled button behavior

[camertron]

What are you trying to accomplish?

This PR updates all of our buttons (via the BaseButton component) to support being disabled. This is done by forcing the tag to <button> if disabled: true is specified.

Screenshots

Screenshots are not necessary in this case because all buttons still appear visually disabled with this change.

Integration

I wrote up a fake ERB linter and Ruby code scanner to identify cases where the disabled: option is passed to Beta::Button, Alpha::ButtonMarketing, Alpha::HellipButton, Beta::IconButton, Beta::CloseButton, Beta::BaseButton, and ButtonComponent, that also specify a tag other than <button>.

The scripts linked above only identified a few usages that fit the criteria:

1. app/components/repos/advisories/report_vulnerability_button_component.html.erb:L1 Strictly speaking, we don't actually have to fix this usage - the code will continue to work as written. No doubt the author discovered what this PR is trying to fix - namely that <a> tags cannot be disabled. That said, we should take the opportunity to clean this up by setting tag: :a.
2. app/components/codespaces/create_options_dropdown_component.html.erb:L7 This one might be tricky because the comments state it needs to be enabled after machine types load. I'm not sure if that's done in javascript or server-side. If done in js, we'll have to change the tag too since it'll be forced to <button>.
3. app/components/reactions/dropdown_component.html.erb. We'll need to test this one since it's inside a <details> tag.
4. app/components/stafftools/sponsors/members/transfers/payment_and_match_reversal_form_component.html.erb. This one should be easy since this button is always rendered disabled. We should be able to simply remove the tag: :summary argument.

List the issues that this change affects.

Fixes https://github.com/github/primer/issues/2222

Risk Assessment

• Low risk the change is small, highly observable, and easily rolled back.
• Medium risk changes that are isolated, reduced in scope or could impact few users. The change will not impact library availability.
• High risk changes are those that could impact customers and SLOs, low or no test coverage, low observability, or slow to rollback.

What approach did you choose and why?

Currently a number of our buttons that allow tag customization can't be disabled if a tag other than <button> is used. This is because the other two types of allowed tag, <a> and <summary>, cannot be disabled by setting the disabled HTML attribute as can be done for <button>s. In the ActionMenu component, we solved this by forcing the tag to <button> if disabled: true is specified. This PR does the same for all users of BaseButton.

Anything you want to highlight for special attention from reviewers?

This change could cause problems with code that assumes a particular tag will be used for any given button. I feel as though I did my due diligence tracking down usages though, so we should be good to go.

Accessibility

• No new axe scan violation - This change does not introduce any new axe scan violations.

This change doesn't fix an Axe violation (that I know of), but it does make our buttons more accessible overall.

Merge checklist

• Added/updated tests
• Added/updated documentation
• Added/updated previews (Lookbook)
• Tested in Chrome
• Tested in Firefox
• Tested in Safari
• Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

[changeset-bot]

🦋 Changeset detected

Latest commit: 84acc81

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/view-components	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[jonrohan]

🤘🏻