Skip to content

Conversation

@jesskuo4
Copy link
Contributor

@jesskuo4 jesskuo4 commented Jun 23, 2023

Describe your changes here.

  • Changed the Leading and Trailing Visual Icon Colors for the Button Component to fg.muted
  • Change ButtonCounter font size to 0 (12px)

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

Screenshots

Before After
Before image of trailing/leading icons After image of trailing/leading icons
Before image of counter with wrong size After image of counter with corrected size

Please provide before/after screenshots for any visual changes

Merge checklist

  • Added/updated tests
  • Added/updated documentation
  • Changes are SSR compatible
  • 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.

@jesskuo4 jesskuo4 requested review from a team and josepmartins June 23, 2023 21:04
@changeset-bot
Copy link

changeset-bot bot commented Jun 23, 2023

🦋 Changeset detected

Latest commit: 84af8ad

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

This PR includes changesets to release 1 package
Name Type
@primer/react 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
Copy link
Contributor

Uh oh! @jesskuo4, 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
Copy link
Contributor

github-actions bot commented Jun 23, 2023

size-limit report 📦

Path Size
dist/browser.esm.js 102.76 KB (+0.02% 🔺)
dist/browser.umd.js 103.31 KB (+0.03% 🔺)

@github-actions
Copy link
Contributor

Uh oh! @jesskuo4, 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.

@jesskuo4 jesskuo4 temporarily deployed to github-pages June 23, 2023 21:11 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3447 June 23, 2023 21:12 Inactive
@github-actions
Copy link
Contributor

Uh oh! @jesskuo4, 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 github-actions bot temporarily deployed to storybook-preview-3447 June 23, 2023 21:19 Inactive
@langermank langermank temporarily deployed to github-pages June 23, 2023 21:20 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3447 June 23, 2023 21:20 Inactive
@langermank langermank added the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Jun 23, 2023
@github-actions github-actions bot removed the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Jun 23, 2023
@primer primer bot temporarily deployed to github-pages June 23, 2023 22:24 Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3447 June 23, 2023 22:24 Inactive
@langermank langermank added the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Jun 23, 2023
@langermank langermank requested a review from maximedegreve June 23, 2023 23:14
@langermank langermank temporarily deployed to github-pages June 23, 2023 23:14 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3447 June 23, 2023 23:15 Inactive
@github-actions github-actions bot removed the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Jun 23, 2023
@primer primer bot temporarily deployed to github-pages June 23, 2023 23:28 Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3447 June 23, 2023 23:28 Inactive
@primer primer deleted a comment from github-actions bot Jun 27, 2023
@jesskuo4
Copy link
Contributor Author

jesskuo4 commented Jun 28, 2023

Boom 🎉

Found two small things:

1️⃣ The icon button example is still not muted.

2️⃣ Then the counter is vertically not aligned centred.

Screenshot 2023-06-24 at 13 19 35

Hi @maximedegreve! 👋 Thank you so much for pointing these two things out and waiting, I wanted to double check some things with my mentor, Katie, before responding:

Regarding num1 of this comment, I was thinking that it might be better to keep the icon button only to remain as fg.default since the current disabled icon button uses the fg.muted look. It might be good to consider keeping it this way so users can differentiate the status of the button. We also have an invisible variant, which has a muted color by default but when it becomes disabled, it will look more like this with a background (its broken right now in PRC). What are your thoughts?
image

Regarding num2, I tried correcting the issue. However, I shared with Katie and she mentioned how to solve the problem, we have to change how the the counter button is layered, which might cause other issues. She is currently looking into this and will get back to you!

Thank you!! 😄

@github-actions
Copy link
Contributor

Uh oh! @jesskuo4, the image you shared is missing helpful alt text. Check #3447 (comment).

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.

2 similar comments
@github-actions
Copy link
Contributor

Uh oh! @jesskuo4, the image you shared is missing helpful alt text. Check #3447 (comment).

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
Copy link
Contributor

Uh oh! @jesskuo4, the image you shared is missing helpful alt text. Check #3447 (comment).

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 langermank temporarily deployed to github-pages July 18, 2023 18:48 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3447 July 18, 2023 18:48 Inactive
@langermank langermank temporarily deployed to github-pages July 18, 2023 18:57 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3447 July 18, 2023 18:57 Inactive
@langermank langermank temporarily deployed to github-pages July 18, 2023 21:39 — with GitHub Actions Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-3447 July 18, 2023 21:39 Inactive
@langermank langermank added the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants