primer / octicons

A scalable set of icons handcrafted with <3 by GitHub
https://primer.style/foundations/icons
MIT License
8.22k stars 829 forks source link

Octicon contribution for RemoveFilter #976

Closed UnicodeRogue closed 10 months ago

UnicodeRogue commented 11 months ago

Closes https://github.com/github/memex/issues/15800

Connected to this Epic: https://github.com/github/primer/issues/2396

I'm working through the contribution guidelines posted here: https://github.com/primer/octicons/blob/6cbce08cb1fde5e316a9a079c2938b7bc80e50f6/CONTRIBUTING.md

Which asks me to make sure to cover:

where the icon will be used:

In the SelectPanel, to indicate removing a filter

any relevant timeline information:

I'm not sure if this refers to the timeline-to-shipping, but I do want to note that as I was adding this icon, I noticed most other icons had their styles written out as a single line (like filter-16.svg, as one example) and I wanted to check if that needed to be the case for new icons as well

changeset-bot[bot] commented 11 months ago

🦋 Changeset detected

Latest commit: eed887c5234dc6ee33f9849ebb727738eb151271

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

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

gr2m commented 11 months ago

Great pull request and description 👍🏼 Based on other icons it seems you'll have to add a 24px variation as well?

No Changeset found

try to run npx changeset and follow instructions

UnicodeRogue commented 11 months ago

Great pull request and description 👍🏼 Based on other icons it seems you'll have to add a 24px variation as well?

ah good call! Added ✔️ and changeset command run ✔️

gr2m commented 11 months ago

looks like you need to update snapshots:

https://github.com/primer/octicons/actions/runs/5904633153/job/16017173213?pr=976#step:8:9

try running npx jest -u

UnicodeRogue commented 11 months ago

note to say I'm continuing to troubleshoot the failing test 🚧

UnicodeRogue commented 10 months ago

running npx jest -u didn't resolve things-

currently running yarn test -u to see how far I can get-

UnicodeRogue commented 10 months ago

Output of npx jest -u

Screenshot 2023-08-22 at 2 25 18 PM
UnicodeRogue commented 10 months ago

Output of running yarn test -u

Screenshot 2023-08-22 at 2 26 34 PM
gavinmn commented 10 months ago

@UnicodeRogue Is there a related Octicons issue for this? I'm trying to understand if this is something the team looked at with you or if this is a new request altogether that we should take a look at in a working session.

gr2m commented 10 months ago

It's a new request, the icon was initially need for https://github.com/github/memex/issues/15800 (private GitHub repository), but will also be needed for the new SelectPanel: https://github.com/github/primer/issues/2396

CameronFoxly commented 10 months ago

Flying in from the octicons working group to help with this....

I think that your icon could use just a tiny bit of adjustment to help the little "x" feel balanced with spacing more consistent with the rest of the set. Here's my proposed adjustment (as well as the 24px version):

Screen Shot 2023-09-01 at 8 53 38 AM

I've commited these into this PR. Let me know if anyone sees any issues here, thanks!

gr2m commented 10 months ago

@primer/octicons-reviewers can we get this pull request merged an released to conclude the work?