primer / brand

React components and Primitives for GitHub marketing websites
https://primer.style/brand
MIT License
74 stars 33 forks source link

Add Icon Component #778

Closed rfearing closed 1 month ago

rfearing commented 1 month ago

Summary

Part one of adding the background around the icon inside the Bento. This adds the Icon component.

List of notable changes:

What should reviewers focus on?

Steps to test:

  1. View Icon component in storybook
  2. Compare to Card.Icon
  3. Examine API to verify it matches other components.

Supporting resources (related issues, external links, etc):

Contributor checklist:

Reviewer checklist:

Screenshots:

Please try to provide before and after screenshots or videos

Before After
changeset-bot[bot] commented 1 month ago

๐Ÿฆ‹ Changeset detected

Latest commit: 86ce2528ed710ab5b869b9712621d9514c732c0b

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

This PR includes changesets to release 6 packages | Name | Type | | ------------------------ | ----- | | @primer/brand-primitives | Patch | | @primer/react-brand | Patch | | @primer/brand-e2e | Patch | | @primer/brand-fonts | Patch | | @primer/brand-config | Patch | | @primer/brand-storybook | Patch |

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[bot] commented 1 month ago

๐ŸŸข No design token changes found

github-actions[bot] commented 1 month ago

โš ๏ธ Visual differences found

Our visual comparison tests found UI differences. Please review the differences by using the test artifacts to ensure that the changes were intentional. Artifacts can be downloaded and reviewed locally. Download links are available at the bottom of the workflow summary screen. ##### Example: ![artifacts section of workflow run](https://user-images.githubusercontent.com/13340707/181026915-2bda8a90-58e3-40ef-a2f6-c9c4af6e9c4a.png) If the changes are expected, please run `npm run test:visual:update-snapshots` to replace the previous fixtures.

Review visual differences

joshfarrant commented 1 month ago

The other thing we'll need, alongside this implementation, is documentation.

That might be dependent on us updating the Figma as we'll likely need some images from Figma to use in the documentation.

Just flagging so we don't forget @danielguillan

rfearing commented 1 month ago

Thank you so much @joshfarrant for the help with prettier and with checking that accessibility! ๐Ÿ‘๐Ÿผ โ™ฟ ! I left a response to the span comments and I think I covered the other suggestions you made.

Thanks, again!

joshfarrant commented 1 month ago

Awesome, thanks for addressing those comments @rfearing ๐Ÿ™Œ

Regarding the span, I had a play around myself and I'm keen to get your thoughts.

I branched off your branch and created a PR against your branch to illustrate the changes

https://github.com/primer/brand/pull/787

I think this is the same approach you mentioned (using padding to achieve the background). I can't see any issues with this approach immediately, but keen to hear what you think.

rfearing commented 1 month ago

@joshfarrant That looked good! Merged it into mine. ๐Ÿ˜„