Updates Button and ButtonMarketing Lookbook previews with accessible examples #2802
Conversation
🦋 Changeset detectedLatest commit: 017fd24 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
| @@ -63,22 +63,18 @@ def icon_buttons(size: :medium) | |||
| end | |||
| end | |||
|
|
|||
| # @label Button group with all tags | |||
| # @label Button group with multiple tags | |||
There was a problem hiding this comment.
I don't know if we'd ever want a summary tag in a button group, so I've removed this example.
Co-authored-by: Kate Higa <16447748+khiga8@users.noreply.github.com>
| @@ -20,7 +20,6 @@ class ButtonPreview < ViewComponent::Preview | |||
| # @param disabled toggle | |||
| # @param inactive toggle | |||
| # @param align_content select [center, start] | |||
| # @param tag select [a, summary, button] | |||
There was a problem hiding this comment.
Since tag is still an option for the component, I think we should at least keep it in the Playground story. This is what we use to test different cases and make sure we haven't broken anything.. I would at least keep it to button, a and maybe there's a way to pass in href if a is selected?
There was a problem hiding this comment.
I can definitely keep it to button and a! There is a way I can dynamically add a # if it is an <a> so I will do that :) Thank you!
Authors: Please fill out this form carefully and completely.
Reviewers: By approving this Pull Request you are approving the code change, as well as its deployment and mitigation plans.
Please read this description carefully. If you feel there is anything unclear or missing, please ask for updates.
What are you trying to accomplish?
These changes make the Lookbook previews accessible and have proper markup. Specifically:
tagargument in the playground<details>elementhrefattributes when a button tag is rendered as an<a>.I've also added documentation with the markup requirements if changing a Button tag to either
summaryora. This does not change any component API or functionality, they are strictly preview changes.One question I had is: should we consider requiring an
hrefto be passed in if the user passes anato thetagargument in the component itself? I didn't want to get too deep into changing component functionality, but I can file a follow-up issue if this is something that's desired.Integration
No
List the issues that this change affects.
Risk Assessment
What approach did you choose and why?
Originally, I tried to render the required attributes (
<details>wrapping asummaryand anhrefattribute for<a>elements) in a conditional way, but it felt messy; while I could get thehrefto render conditionally, we needed to add it to every preview example which was not ideal. I also couldn't figure out how to conditionally wrap the button previews with a<details>element, which I don't think is possible in Ruby (but I could be wrong, open to other ideas here!)I felt the best option was to remove the option to change the tag entirely. It doesn't affect anything visually, and we don't want to give people the wrong impression about how to use the component so I broke things out into their own stories (i.e. Summary as button and Link as button, the latter already existed). I also added an
hrefparam in the previews where the buttons are rendered as link tags.Anything you want to highlight for special attention from reviewers?
Are there other approaches you'd suggest here? I am a newer Primer contributor so all feedback is welcomed ✨
Merge checklist
Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.