Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add pause icon #1070

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Add pause icon #1070

wants to merge 13 commits into from

Conversation

smockle
Copy link
Member

@smockle smockle commented Feb 13, 2025

I reviewed and followed the steps in “Adding or updating an icon”.

This PR adds a new pause icon (represented by parallel vertical lines).

GitHub uses a similar icon in two places:

@smockle smockle requested review from a team as code owners February 13, 2025 19:58

This comment was marked as resolved.

@smockle
Copy link
Member Author

smockle commented Feb 13, 2025

The CI / npm:@primer/octicons-react (pull_request) check is failing with:

Snapshot name: @primer/octicons-react should not update exports without a semver change 1

@CameronFoxly @TylerJDev (reviewers) — Could y’all please advise on how to resolve this? Should I push a commit that bumps the version number in package.json? If so, would this be considered a minor version bump?

@TylerJDev
Copy link

Hey @smockle! I think for npm:@primer/octicons-react, we need to add the icon to public-api.test.js.snap. I think this can be done locally via yarn test -u, which should produce an update to that file.

@smockle
Copy link
Member Author

smockle commented Feb 18, 2025

@TylerJDev Thanks! I updated the snapshot in b2af2e0; that fixed the lint issue.

There’s a new failure in an optional check, but it isn’t one introduced by my PR. It’s failing in main too. #1072 should fix it.

<svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24"><path d="M12 1C5.923 1 1 5.923 1 12c0 4.867 3.149 8.979 7.521 10.436.55.096.756-.233.756-.522 0-.262-.013-1.128-.013-2.049-2.764.509-3.479-.674-3.699-1.292-.124-.317-.66-1.293-1.127-1.554-.385-.207-.936-.715-.014-.729.866-.014 1.485.797 1.691 1.128.99 1.663 2.571 1.196 3.204.907.096-.715.385-1.196.701-1.471-2.448-.275-5.005-1.224-5.005-5.432 0-1.196.426-2.186 1.128-2.956-.111-.275-.496-1.402.11-2.915 0 0 .921-.288 3.024 1.128a10.193 10.193 0 0 1 2.75-.371c.936 0 1.871.123 2.75.371 2.104-1.43 3.025-1.128 3.025-1.128.605 1.513.221 2.64.111 2.915.701.77 1.127 1.747 1.127 2.956 0 4.222-2.571 5.157-5.019 5.432.399.344.743 1.004.743 2.035 0 1.471-.014 2.654-.014 3.025 0 .289.206.632.756.522C19.851 20.979 23 16.854 23 12c0-6.077-4.922-11-11-11Z"/></svg>
Copy link

@TylerJDev TylerJDev Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this changed, not sure if this was intentional 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like that was changed by a bot in 0e6838e 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reverted the bot’s commit in d443aaa. I’m curious if it’ll re-add it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, the bot re-added the commit: 757444b. 🤷

@TylerJDev
Copy link

My assumption is that it's caused by this PR: #1050. I'm guessing the icon didn't go through the "optimization" step, which is why it's being optimized in this PR. Overall, I don't see a difference, so I think it's fine.

Copy link

@TylerJDev TylerJDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

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.

3 participants