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

Update Octicons #65989

Merged
merged 10 commits into from Jan 8, 2019
Merged

Update Octicons #65989

merged 10 commits into from Jan 8, 2019

Conversation

miguelsolorio
Copy link
Contributor

@miguelsolorio miguelsolorio commented Jan 3, 2019

Fixes #65508 and fixes #36053

This updates our version of Octicons, while still having backward compatibility with icons that have been removed in the newer versions. I've created this tool that generates the new icon from the Octicon npm module.

The only minor changes are that organization and person are now filled versions instead of outlines.

Additions

image

  • arrow-both
  • bold
  • eye-closed
  • fold-down
  • fold-up
  • grabber
  • italic
  • kebab-horizontal
  • kebab-vertical
  • logo-gist
  • note
  • plus-small
  • project
  • screen-full
  • screen-normal
  • smiley
  • tasklist
  • text-size
  • unverified
  • verified

Updated

image

  • color-mode is persisted
  • organization-filled is for the new version of the icon
  • person-filled is for the new version of the icon

Comparison

After installing a few extensions that use octicons in the statusbar (vscode-spotify, live share, github pull request), you can compare the differences.

image

@miguelsolorio miguelsolorio added the ux User experience issues label Jan 3, 2019
@miguelsolorio miguelsolorio added this to the December/January 2019 milestone Jan 3, 2019
@bpasero
Copy link
Member

bpasero commented Jan 4, 2019

@misolori very cool. some thoughts:

Here is a screenshot of the new icons we get:

image

I feel like some we should maybe revisit (maybe exclude?), especially:

  • the ones that show up very small (e.g. the + icon)
  • the "Gist" one does not make a lot of sense imho.

Otherwise, I would suggest that we maybe keep those icons that change a lot (e.g. the people icon for live share) and have the new one under a different name?

@bpasero bpasero assigned miguelsolorio and unassigned bpasero Jan 4, 2019
@bpasero bpasero self-requested a review January 4, 2019 09:26
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

Approving, the change looks to me good otherwise.

@bpasero
Copy link
Member

bpasero commented Jan 4, 2019

@misolori just noticed two things:

  • please keep in mind to release note this properly, I would also think we could create an issue over in vscode-docs to document the valid icons a bit better (maybe with showing the icon on the website?)
  • run a search for octicon in our repo to see where we use octicons in our core and verify nothing is broken, e.g. it looks like in some places we directly reference an icon through CSS
  • finally it would be good to run a full build with your changes just to make sure things are packaged up properly. e.g. I noticed some build rules in our gulpfiles related to octicons

@miguelsolorio
Copy link
Contributor Author

miguelsolorio commented Jan 4, 2019

@bpasero:

  • Removing the logo-gist and plus-small is totally fine. I've seen people not wanting the Gist logo in other issues and we already have a regular plus.
  • I can do a swap of the people and organization icons for Live Share and move the new ones to the appended name
  • I was already planning on mentioning this in the release notes, I've created Document changes to Octicons vscode-docs#2315 to track this is
  • I'll also do a final test and double-check the other areas Octicons are being used and create a custom build

👍 Thanks for feedback!

Miguel Solorio added 2 commits January 4, 2019 10:47
Making the previous icons of "organization" and "person" the default and appending "filled" to the newer versions. Also removing "logo-gist" and "plus-small" as they are not needed.
@bpasero
Copy link
Member

bpasero commented Jan 5, 2019

@misolori great, feel free to merge then after you tested the build

@miguelsolorio
Copy link
Contributor Author

miguelsolorio commented Jan 7, 2019

Fixed a rendering bug that appeared in Windows, still need to test on Linux and then will merge.

@miguelsolorio
Copy link
Contributor Author

Verified the changes on macOS/Win10/Linux and it all looks good, will merge this in.

@miguelsolorio miguelsolorio merged commit 5df3154 into master Jan 8, 2019
@miguelsolorio miguelsolorio deleted the misolori/octicon-update branch January 8, 2019 18:02
@miguelsolorio miguelsolorio mentioned this pull request Jan 8, 2019
3 tasks
@github-actions github-actions bot locked and limited conversation to collaborators Mar 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
ux User experience issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Octicons Please update octicons from 3.1.0 to 6.0.1
2 participants