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

Inline all SVGs for the workbench when building #8516

Closed
bpasero opened this issue Jun 29, 2016 · 5 comments
Closed

Inline all SVGs for the workbench when building #8516

bpasero opened this issue Jun 29, 2016 · 5 comments
Assignees
Labels
debt Code quality issues
Milestone

Comments

@bpasero
Copy link
Member

bpasero commented Jun 29, 2016

Note: one issue we have is that many SVG icons contain metadata or unused CSS classes.

  1. look into where SVG icons are duplicated in the CSS file and move them to one selector
  2. Go through all our CSS and remove duplicate references to the same svg file
  3. Go to gulpfile.common.js and add result['vs/css'] = { inlineResources: true }; at https://github.com/Microsoft/vscode/blob/master/build/gulpfile.common.js#L48

To fix #7839

@bpasero bpasero added the debt Code quality issues label Jun 29, 2016
@bpasero bpasero added this to the July 2016 milestone Jun 29, 2016
@bpasero bpasero self-assigned this Jun 29, 2016
@bpasero
Copy link
Member Author

bpasero commented Jul 6, 2016

@bgashler1 we have many places in code where the same SVG is referenced in a rule with background and content (e.g. https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/browser/parts/editor/media/titlecontrol.css#L114 and https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/browser/parts/editor/media/titlecontrol.css#L126).

Do you recall why for HC theme we do this? If we inline all SVGs, we end up duplicating the data URI value if we reference the image multiple times. Maybe there is a way to avoid this and still have a good looking HC theme?

@bpasero
Copy link
Member Author

bpasero commented Jul 6, 2016

Interesting, on Mac comparing the 2 application bundles I see this:

  • inlined version: 152.424.577 bytes (158,9 MB on disk)
  • normal version: 152.386.366 bytes (159,7 MB on disk)

So, in raw bytes we loose, but in terms of allocated disk space we win :)

@bgashler1
Copy link
Contributor

@bpasero:

Removing junk metadata and unused classes I think can be done automatically with this or another gulp package https://www.npmjs.com/package/gulp-svgmin

The reason why we historically started doing pseudo elements with content is because of Windows' high-contrast mode. When in high-contrast mode, Edge and Internet Explorer both remove all background images (thus all of our icons that are used in background images, which is most). To get around this we used the pseudo elements to insert images inline via content. See this issue #3690 for more info and for a media query solution that would make it so would no longer need to do anything special for high-contrast theme to make icons display in background.

@bpasero
Copy link
Member Author

bpasero commented Jul 9, 2016

@bgashler1 I commented on that issue, imho we should make this change and delete all our custom HC rules. This would not only enable this issue for inlining but also make it much easier to introduce new icons to Code because you don't have to deal with this ugly hack.

@bpasero bpasero changed the title Explore to inline all SVGs for the workbench when building Inline all SVGs for the workbench when building Jul 11, 2016
@bpasero
Copy link
Member Author

bpasero commented Jul 11, 2016

Pushed to master. I got rid of our high contrast CSS hacks for background image.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues
Projects
None yet
Development

No branches or pull requests

2 participants