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.

Sign up for GitHub

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

Jump to bottom

Pushed revisions for the settings icons #715

Merged
merged 11 commits into from
Jan 25, 2022
Merged

Pushed revisions for the settings icons #715

merged 11 commits into from
Jan 25, 2022

Conversation

edokoa
Copy link
Contributor

@edokoa edokoa commented Jan 7, 2022 •
edited
Loading

As discussed in: https://github.com/github/primer/issues/486

Icons are still pending of discussion with the whole team but are a blocker right now. We will review again later.

image

https://www.figma.com/file/2rjsQp42uKvzPXzLZIqOeA/Octicons-v2---Source?node-id=7071%3A51354

Mock of a menu with all the icons

image

edokoa and others added 2 commits January 7, 2022 15:23
@edokoa
Save pages-16.svg, id-badge-16.svg, webhook-16.svg, log-16.svg, tab-e…
549ddbb 
…xternal-16.svg, repo-deleted-16.svg, apps-16.svg, accessibility-16.svg
@actions-user
Optimize SVGs
c5ee4e5
@edokoa edokoa requested a review from a team as a code owner January 7, 2022 14:25
@edokoa edokoa requested a review from Juliusschaeper January 7, 2022 14:25
@changeset-bot
Copy link

changeset-bot bot commented Jan 7, 2022 •
edited
Loading

⚠️ No Changeset found

Latest commit: d7c6972

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

edokoa
edokoa commented Jan 7, 2022
View reviewed changes
Copy link
Contributor Author

@edokoa edokoa left a comment

Choose a reason for hiding this comment

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

Checked transparent backgrounds

@edokoa edokoa changed the title Pushed revisions for the NavigationList icons Pushed revisions for the settings icons Jan 7, 2022
@edokoa edokoa requested review from ashygee and vdepizzol January 7, 2022 14:56
@edokoa
Copy link
Contributor Author

edokoa commented Jan 7, 2022

cc @alliethu and @ichelsea as this includes retouching the accessibility icon.

@edokoa edokoa requested review from alliethu and ichelsea January 7, 2022 15:01
@edokoa edokoa added the octicon label Jan 7, 2022
vdepizzol
vdepizzol approved these changes Jan 10, 2022
View reviewed changes
Copy link

@vdepizzol vdepizzol left a comment

Choose a reason for hiding this comment

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

✨ ✨ ✨

vdepizzol
vdepizzol reviewed Jan 10, 2022
View reviewed changes
icons/log-16.svg Outdated
@@ -0,0 +1 @@
<svg xmlns="http://www.w3.org/2000/svg" width="16" height="17" viewBox="0 0 16 17"><path d="M5 9.25a.75.75 0 01.75-.75h4a.75.75 0 010 1.5h-4A.75.75 0 015 9.25zM4 11.5A.75.75 0 004 13h4a.75.75 0 000-1.5H4z"/><path fill-rule="evenodd" d="M13 .995H3a3 3 0 00-3 3c0 .676.224 1.254.603 1.722.526.65 1.331.783 1.907.783h1.177c-.364.662-.814 1.339-1.287 2.048-.205.309-.414.624-.623.946C.891 10.865 0 12.418 0 14a3 3 0 003 3h10a3 3 0 001.667-5.494.75.75 0 00-.834 1.246A1.5 1.5 0 1111.5 14c0-.642.225-1.347.623-2.136.397-.787.933-1.593 1.501-2.446l.011-.017c.554-.83 1.139-1.709 1.582-2.588.445-.885.783-1.836.783-2.818 0-1.672-1.346-3-3-3zm-10 1.5a1.5 1.5 0 00-1.5 1.5c0 .321.1.569.27.778.097.12.325.227.74.227h7.674A2.737 2.737 0 0110 3.995c0-.546.146-1.059.401-1.5H3zm10 0c.831 0 1.5.662 1.5 1.5 0 .646-.225 1.353-.623 2.143-.398.79-.933 1.595-1.501 2.448l-.017.026c-.552.828-1.134 1.702-1.575 2.576C10.338 12.072 10 13.021 10 14c0 .546.146 1.059.401 1.5H3A1.5 1.5 0 011.5 14c0-1.084.63-2.289 1.537-3.692.177-.274.366-.556.558-.845.632-.948 1.306-1.96 1.773-2.963h6.382a.75.75 0 00.417-1.373c-.444-.298-.667-.656-.667-1.132a1.5 1.5 0 011.5-1.5z"/></svg>
Copy link

Choose a reason for hiding this comment

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

@edokoa this file is using a height of 17 in the height and viewBox attributes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw Figma doing something strange with another icon and trying to push it as 15.99999999999999999 even if it was 16 in the interface. I'll re-upload this one. Thanks for catching this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for not updating here, but this was corrected in f48e121

ichelsea
ichelsea approved these changes Jan 10, 2022
View reviewed changes
Copy link

@ichelsea ichelsea left a comment

Choose a reason for hiding this comment

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

I'm comfortable with where the a11y icon has landed! What's odd for me is it's different than the accessibility icon than I'm used to seeing but I think it still represents what we're going for and it's cute! I don't have any recommendations for what to do differently so I think this icon is in a good spot to go live.

@emilybrick
Copy link
Contributor

The accessibility icon is reading to me as a stick figure or illustration of a stick figure more than it is a representation for accessibility. Can we keep working on this one without blocking the others? think it may need another pass.

@tallys
Copy link
Contributor

tallys commented Jan 11, 2022 •
edited
Loading

Hi @edokoa ! Asked some accessibility folks about the icons - we will see what the feedback is. My initial take was reading it as more "person" and not "accessibility". Perhaps for me what signals it being accessibility is a T shaped stick figure (not arms down) and probably standing straight up and not having the A-frame shape. Overall scroll is getting positive feedback, but I'm not too familiar with the history of scrolls at GitHub :)

@adrianmg
Copy link
Contributor

adrianmg commented Jan 11, 2022 •
edited
Loading

Thanks for sharing, Javi!! Dropping some feedback regarding Pages icon:

  • I'm finding its shape too similar to the copy-to-clipboard one.
  • I wonder if we could explore a different concept. At the moment, it feels like a direct translation from GitHub Pages to Pages from a document (or files/wiki) that doesn't resonate much with the service itself.

image

@edokoa
Copy link
Contributor Author

edokoa commented Jan 18, 2022 •
edited
Loading

@emilybrick, @ichelsea, @alliethu: I actually worked on a version of the accessibility icon with straight arms (which was a cleanup of the one @ashygee designed). I just tilted the arms down to make it feel a little bit more organic. I can give it another go from this one, which was before bending the arms.

image

Figma

@ashygee
Copy link
Contributor

ashygee commented Jan 18, 2022

I wonder if we could explore a different concept. At the moment, it feels like a direct translation from GitHub Pages to Pages from a document (or files/wiki) that doesn't resonate much with the service itself.

@adrianmg thank you for pointing that out. We are going to move forward with recommending the current browser icon to be used for the GitHub Pages link.

We spoke with @samoshin from our Brand team and we will move forward with the current logs icon (scroll) for now with room to iterate at a later time.

For the accessibility icon we're going to move with the current proposal as there was no definitive feedback that there was anything conflicting with this version. This is the first iteration of the accessibility icon which is needed within Octicons and we can iterate later if we formulate more of an opinion as to how we want to visualize accessilibity.

@edokoa
Copy link
Contributor Author

edokoa commented Jan 18, 2022

I'm going to push this one. I feel the arms are 1px too low in the previous version.

image

Figma

actions-user and others added 2 commits January 18, 2022 18:04
@actions-user
Optimize SVGs
000d85e
@edokoa
Delete pages-16.svg
38cdb42 
Deleting the 'pages' icon as we're going with the already existing 'browser' icon.
@ashygee ashygee merged commit ab991ab into main Jan 25, 2022
@ashygee ashygee deleted the settings-octicons-revision branch January 25, 2022 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vdepizzol vdepizzol vdepizzol approved these changes

@ichelsea ichelsea ichelsea approved these changes

@ashygee ashygee ashygee approved these changes

@Juliusschaeper Juliusschaeper Awaiting requested review from Juliusschaeper Juliusschaeper is a code owner automatically assigned from primer/octicons-reviewers

@alliethu alliethu Awaiting requested review from alliethu

Assignees
No one assigned
Labels
octicon
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@edokoa @emilybrick @tallys @adrianmg @ashygee @vdepizzol @ichelsea @actions-user @colebemis