-
Notifications
You must be signed in to change notification settings - Fork 829
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
Octicon contribution for RemoveFilter #976
Conversation
🦋 Changeset detectedLatest commit: eed887c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
b5f461c
to
b4fd0af
Compare
b4fd0af
to
19e8220
Compare
Great pull request and description 👍🏼 Based on other icons it seems you'll have to add a 24px variation as well?
try to run |
ah good call! Added ✔️ and changeset command run ✔️ |
looks like you need to update snapshots: https://github.com/primer/octicons/actions/runs/5904633153/job/16017173213?pr=976#step:8:9 try running |
note to say I'm continuing to troubleshoot the failing test 🚧 |
running currently running |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the 24px SVG needs to be updated to use up the full space, right now it is still rendered as 16px with the rest being empty space. I'm not sure if there is an easy hack to upscale a 16px version? I'm sure the @primer/octicons-reviewers team will be able to help to give some pointers 🙏🏼
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I'm checking with Design!
@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. |
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 |
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): I've commited these into this PR. Let me know if anyone sees any issues here, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you @CameronFoxly - it looks great!
@primer/octicons-reviewers can we get this pull request merged an released to conclude the work? |
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:
In the SelectPanel, to indicate removing a filter
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