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

Please update octicons from 3.1.0 to 6.0.1 #36053

Closed
rwnkl opened this issue Oct 11, 2017 · 11 comments · Fixed by #65989
Closed

Please update octicons from 3.1.0 to 6.0.1 #36053

rwnkl opened this issue Oct 11, 2017 · 11 comments · Fixed by #65989
Assignees
Labels
debt Code quality issues ux User experience issues

Comments

@rwnkl
Copy link

rwnkl commented Oct 11, 2017

No description provided.

@vscodebot vscodebot bot added the install-update VS Code installation and upgrade system issues label Oct 11, 2017
@egamma egamma removed the install-update VS Code installation and upgrade system issues label Oct 11, 2017
@jrieken jrieken added under-discussion Issue is under discussion for relevance, priority, approach ux User experience issues labels Oct 11, 2017
@jrieken
Copy link
Member

jrieken commented Oct 11, 2017

Unfortunately not so easy because some octicons have been removed and hence updating might likely break some extensions that rely on them

@Gruntfuggly
Copy link

Is there a plan going forwards with this? I just tried to use an octicon and it came up blank. I couldn't work out why until I checked the versions being used, which led me here.

From the change log, it looks like some of the removed icons are just renamed. Would it be possible to add a simple 'redirect' to return the new icon if the old name is used? Similarly, if an icon has genuinely been removed, could a placeholder be returned? In fact, it would probably be nicer to return something if a bad icon is named, rather than leaving a blank space.

@jrieken
Copy link
Member

jrieken commented May 16, 2018

would it be possible to add a simple 'redirect' to return the new icon if the old name is used?

Hm... Do you have a pointer/release notes that describe these changes?

@Gruntfuggly
Copy link

I was just looking through here:
https://github.com/primer/octicons/blob/master/CHANGELOG.md

@jrieken jrieken added the debt Code quality issues label May 16, 2018
@jrieken
Copy link
Member

jrieken commented May 16, 2018

Yeah, so I believe we would miss two icons and we might wanna risk that... For the other we have alternatives in place... No promises, stretch for June (also accepting PRs)

@jrieken jrieken added this to the June 2018 milestone May 16, 2018
@Gruntfuggly
Copy link

Great - thanks!

@jrieken
Copy link
Member

jrieken commented Jun 5, 2018

So, this would be the compat table and the good news is that no extension seems to use those three removed icons. The bad news is that octicons are now shipped as npm module which we cannot consume at the level at which we define the OcticonLabel

const _compatMapping = {
	// https://github.com/primer/octicons/blob/master/CHANGELOG.md#320-november-6-2015
	'screen-normal': '',
	'screen-full': '',
	// https://github.com/primer/octicons/blob/master/CHANGELOG.md#340-january-22-2016
	'color-mode': '',
	// https://github.com/primer/octicons/blob/master/CHANGELOG.md#600
	'ellipses': 'ellipsis',
	//https://github.com/primer/octicons/blob/master/CHANGELOG.md#601
	'kebab-veritcal': 'kebab-vertical',
	// https://github.com/primer/octicons/blob/master/CHANGELOG.md#octicons_node-700
	'file-text': 'file',
	'mail-reply': 'reply'
};

@jrieken jrieken removed this from the June 2018 milestone Jun 5, 2018
@bpasero
Copy link
Member

bpasero commented Jun 5, 2018

@jrieken we already have all the octicon resources checked in to our sources, so why is the fact that Octicons now ship as an NPM module an issue for us moving forward? Can we not just do the same?

image

@jrieken
Copy link
Member

jrieken commented Jun 5, 2018

Yeah, copying the node-module isn't the blocker, tho weird, the issue is more that it's not a font anymore but 150+ svg-files which we need to bundle in a clever way.

@bpasero
Copy link
Member

bpasero commented Jun 6, 2018

Maybe @misolori has an idea on how to do that. I wonder if we can convert the set back to a font so that it is easy for us to consume it as we did before.

@miguelsolorio
Copy link
Contributor

@chryw and I talked about how to convert the new svgs back into an icon font. We'd need to setup a project (probably in node) that would have the right modules to convert svgs into icons. @chryw already has a similar project but we'd need to:

  • Format the octicons file names to include their unicode
  • Format the less/sass templates to match the old styles format
  • Audit the changes from the old version of Octicons (renames/deletions/additions)
  • Map all of the changes
  • Also create local ttf file for users to install

This would take some time to setup, but nonetheless it is very hacky. Github has already moved towards using svgs so this would be a patch that we'd need to maintain.

One last thing to consider is if in the future we decide to create our own icon set (related to #8017) then this would be deprecated. I'd be in favor of putting our efforts into creating our own icon set that can be consumed by everyone.

@jrieken jrieken removed the under-discussion Issue is under discussion for relevance, priority, approach label Sep 12, 2018
@miguelsolorio miguelsolorio mentioned this issue Dec 20, 2018
6 tasks
@bpasero bpasero assigned miguelsolorio and unassigned jrieken Jan 4, 2019
@bpasero bpasero added this to the December/January 2019 milestone Jan 4, 2019
@vscodebot vscodebot bot locked and limited conversation to collaborators Feb 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues ux User experience issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants