Support with_leading_visual_svg in Primer::Beta::Button #2215

mjimenez98 · 2023-08-18T17:18:42Z

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?

Just like ActionList currently supports, I would like to have a SVG serve as the leading visual icon in a Primer::Beta::Button component. At the moment only Octicons are allowed.

Screenshots

Picture of the button component with a SVG as a leading icon:

Also, before/after pictures from changes to Lookbook:

Before	After

Integration

List the issues that this change affects.

Closes https://github.com/github/primer/issues/2565

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?

Extend the leading_visual function in Primer::Beta::Button to support SVGs. I used the same pattern found in ActionList.

Here is the generated HTML for the example found in the Lookbook:

<button id="button-with-leading-svg-preview" type="button" data-view-component="true" class="Button--invisible Button--medium Button">    <span class="Button-content">
    <span class="Button-visual Button-leadingVisual">
      <svg width="16" height="16" data-view-component="true">    <path d="M8 16a2 2 0 001.985-1.75c.017-.137-.097-.25-.235-.25h-3.5c-.138 0-.252.113-.235.25A2 2 0 008 16z"></path><path fill-rule="evenodd" d="M8 1.5A3.5 3.5 0 004.5 5v2.947c0 .346-.102.683-.294.97l-1.703 2.556a.018.018 0 00-.003.01l.001.006c0 .002.002.004.004.006a.017.017 0 00.006.004l.007.001h10.964l.007-.001a.016.016 0 00.006-.004.016.016 0 00.004-.006l.001-.007a.017.017 0 00-.003-.01l-1.703-2.554a1.75 1.75 0 01-.294-.97V5A3.5 3.5 0 008 1.5zM3 5a5 5 0 0110 0v2.947c0 .05.015.098.042.139l1.703 2.555A1.518 1.518 0 0113.482 13H2.518a1.518 1.518 0 01-1.263-2.36l1.703-2.554A.25.25 0 003 7.947V5z"></path>
      </svg>
    </span>
    <span class="Button-label">Leading SVG button</span>
  </span>
</button>

Anything you want to highlight for special attention from reviewers?

N/A

Accessibility

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

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.

changeset-bot · 2023-08-18T17:18:45Z

🦋 Changeset detected

Latest commit: bb34249

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

github-actions · 2023-08-18T17:18:57Z

Uh oh! @mjimenez98, the image you shared is missing helpful alt text. Check your pull request body.

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

github-actions · 2023-08-18T17:22:24Z

Uh oh! @mjimenez98, the image you shared is missing helpful alt text. Check your pull request body.

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

github-actions · 2023-08-18T17:23:08Z

⚠️ Visual differences found

Our visual comparison tests found UI differences. Please review the differences by viewing the files changed tab to ensure that the changes were intentional.

Review visual differences

into mj98-pri-bls

github-actions · 2023-08-18T17:29:33Z

Uh oh! @mjimenez98, the image you shared is missing helpful alt text. Check your pull request body.

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

github-actions · 2023-08-18T17:30:43Z

Uh oh! @mjimenez98, the image you shared is missing helpful alt text. Check your pull request body.

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

github-actions · 2023-08-18T17:31:42Z

Uh oh! @mjimenez98, the image you shared is missing helpful alt text. Check your pull request body.

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

github-actions · 2023-08-18T17:39:15Z

Uh oh! @mjimenez98, the image you shared is missing helpful alt text. Check your pull request body.

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

github-actions · 2023-08-18T17:39:59Z

Uh oh! @mjimenez98, the image you shared is missing helpful alt text. Check your pull request body.

Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image.

Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.

langermank · 2023-08-18T18:03:46Z

previews/primer/beta/button_preview/leading_visual.html.erb

+  Leading icon button
+<% end %>
+
+<%= render(Primer::Beta::Button.new(


Would you mind actually creating a new preview for the SVG version so we can isolate the behavior of each? 🙌

For sure! Done in e6dd38f

langermank · 2023-08-18T18:09:05Z

One thing I'm noticing is that a custom SVG won't necessarily access the correct colors defined by the Button component CSS. Octicons have currentColor set as fill so it will inherit, but in the example SVG you used it doesn't have that. I see two paths:

Update the CSS to target any svg elements inside the button visual class
Assume all custom SVGs will set the fill to currentColor.

What do you think?

mjimenez98 · 2023-08-21T21:17:03Z

@langermank Great catch! Thank you!

I like the idea of matching the functionality of leading_with_icon_svg to that of leading_with_icon 👍

Update the CSS to target any svg elements inside the button visual class

I gave this a try in b7aedb4. Let me know what you think!

I noticed that the same issue was happening with ActionList so I also tried to fix the color issue for it in b7aedb4

into mj98-pri-bls

jonrohan

This looks good! Thanks for the contribution ✨

langermank

Looks great, thanks! 🚀

Co-authored-by: mjimenez98 <mjimenez98@users.noreply.github.com>

mjimenez98 added 4 commits August 18, 2023 15:41

Add leading_visual_svg to button component

185490b

add with_leading_visual_svg example to lookbook

113d4e7

use more descriptive labels for buttons

33c3f51

add test

2c7ea67

mjimenez98 had a problem deploying to preview August 18, 2023 17:18 — with GitHub Actions Error

github-actions bot added the ruby Pull requests that update Ruby code label Aug 18, 2023

Generating component snapshots

f9592c4

mjimenez98 added 2 commits August 18, 2023 17:26

fix ids for buttons in preview

89904f9

Merge branch 'mj98-pri-bls' of https://github.com/primer/view_components

65bb542

into mj98-pri-bls

mjimenez98 temporarily deployed to preview August 18, 2023 17:27 — with GitHub Actions Inactive

add changeset

322b533

mjimenez98 temporarily deployed to preview August 18, 2023 17:46 — with GitHub Actions Inactive

github-actions bot added the minor release label Aug 18, 2023

mjimenez98 marked this pull request as ready for review August 18, 2023 17:50

mjimenez98 requested review from a team and camertron August 18, 2023 17:50

langermank reviewed Aug 18, 2023

View reviewed changes

mjimenez98 added 2 commits August 21, 2023 16:00

create new preview for button with leading svg icon

e6dd38f

fill leading svg icon with current color for button and action_list

b7aedb4

mjimenez98 requested a review from a team as a code owner August 21, 2023 21:12

mjimenez98 requested a review from langermank August 21, 2023 21:12

mjimenez98 had a problem deploying to preview August 21, 2023 21:13 — with GitHub Actions Error

github-actions bot added the css Pull requests that update CSS code label Aug 21, 2023

mjimenez98 and others added 3 commits August 21, 2023 21:17

Generating component snapshots

41121e8

fix stylelint error -- make currentColor lowercase

c792800

Merge branch 'mj98-pri-bls' of https://github.com/primer/view_components

bb34249

into mj98-pri-bls

mjimenez98 deployed to preview August 21, 2023 21:22 — with GitHub Actions View deployment

jonrohan approved these changes Aug 21, 2023

View reviewed changes

langermank approved these changes Aug 22, 2023

View reviewed changes

jonrohan merged commit 827634e into main Aug 22, 2023
27 checks passed

jonrohan deleted the mj98-pri-bls branch August 22, 2023 17:48

jonrohan temporarily deployed to preview August 22, 2023 17:48 — with GitHub Actions Inactive

primer bot mentioned this pull request Aug 22, 2023

Release Tracking #2219

Merged

jonrohan pushed a commit that referenced this pull request Aug 22, 2023

Support with_leading_visual_svg in Primer::Beta::Button (#2215)

b46897b

Co-authored-by: mjimenez98 <mjimenez98@users.noreply.github.com>

openprojectci mentioned this pull request Sep 4, 2023

Release Tracking opf/primer_view_components#21

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support with_leading_visual_svg in Primer::Beta::Button #2215

Support with_leading_visual_svg in Primer::Beta::Button #2215

mjimenez98 commented Aug 18, 2023 •

edited

Loading

changeset-bot bot commented Aug 18, 2023 •

edited

Loading

github-actions bot commented Aug 18, 2023

github-actions bot commented Aug 18, 2023

github-actions bot commented Aug 18, 2023 •

edited

Loading

github-actions bot commented Aug 18, 2023

github-actions bot commented Aug 18, 2023

github-actions bot commented Aug 18, 2023

github-actions bot commented Aug 18, 2023

github-actions bot commented Aug 18, 2023

langermank Aug 18, 2023

mjimenez98 Aug 21, 2023

langermank commented Aug 18, 2023

mjimenez98 commented Aug 21, 2023

jonrohan left a comment

langermank left a comment

Support with_leading_visual_svg in Primer::Beta::Button #2215

Support with_leading_visual_svg in Primer::Beta::Button #2215

Conversation

mjimenez98 commented Aug 18, 2023 • edited Loading

What are you trying to accomplish?

Screenshots

Integration

List the issues that this change affects.

Risk Assessment

What approach did you choose and why?

Anything you want to highlight for special attention from reviewers?

Accessibility

Merge checklist

changeset-bot bot commented Aug 18, 2023 • edited Loading

🦋 Changeset detected

github-actions bot commented Aug 18, 2023

github-actions bot commented Aug 18, 2023

github-actions bot commented Aug 18, 2023 • edited Loading

⚠️ Visual differences found

github-actions bot commented Aug 18, 2023

github-actions bot commented Aug 18, 2023

github-actions bot commented Aug 18, 2023

github-actions bot commented Aug 18, 2023

github-actions bot commented Aug 18, 2023

langermank Aug 18, 2023

Choose a reason for hiding this comment

mjimenez98 Aug 21, 2023

Choose a reason for hiding this comment

langermank commented Aug 18, 2023

mjimenez98 commented Aug 21, 2023

jonrohan left a comment

Choose a reason for hiding this comment

langermank left a comment

Choose a reason for hiding this comment

mjimenez98 commented Aug 18, 2023 •

edited

Loading

changeset-bot bot commented Aug 18, 2023 •

edited

Loading

github-actions bot commented Aug 18, 2023 •

edited

Loading