Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Optimise SVG (WIP) #6

Closed
kud opened this Issue Jul 18, 2014 · 17 comments

Comments

Projects
None yet
3 participants

kud commented Jul 18, 2014

We need to optimise svg-symbols.svg. It shouldn't have <desc> tag for SEO context.

(bad) example:

image

Just a memo for a future PR.

var gulp       = require('gulp')

var through2   = require('through2')

var plumber    = require('gulp-plumber')
var svgSymbols = require('gulp-svg-symbols')
var filter     = require('gulp-filter')
var imagemin   = require('gulp-imagemin')

module.exports = function() {

  var cssFilter = filter('**/*.css')
  var svgFilter = filter('**/*.svg')

  return gulp.src('src/frontend/glyphicons/**/*.svg')
    .pipe( plumber() )
    .pipe( through2.obj(function ( file, enc, cb ) {

      var fileString = file.contents.toString()

      var titleRegex = /<title>.*<\/title>/gi
      var descRegex = /<desc>.*<\/desc>/gi
      var commentRegex = /<!--.*-->/gi
      var defsRegex = /<defs>.*<\/defs>/gi
      var mSShapeGroupRegex = / +sketch:type=\"MSShapeGroup\"/gi
      var mSPageRegex = / +sketch:type=\"MSPage\"/gi

      fileString = fileString.replace(titleRegex, '')
      fileString = fileString.replace(descRegex, '')
      fileString = fileString.replace(commentRegex, '')
      fileString = fileString.replace(defsRegex, '')
      fileString = fileString.replace(mSShapeGroupRegex, '')
      fileString = fileString.replace(mSPageRegex, '')

      file.contents = new Buffer( fileString )

        this.push( file )
        cb()

    }) )
    .pipe(
      svgSymbols({
        svgId: 'pmd-Svg--%f',
        className: '.pmd-Svg--%f',
        fontSize: 32,
        css: true
      })
    )
    .pipe( cssFilter )
    .pipe( gulp.dest('build') )
    .pipe( cssFilter.restore() )
    .pipe( svgFilter )
    .pipe( gulp.dest('web/assets/images') )

}

We also should work on dest folder for svg and css. It should be an option, not a filter like I did.

Owner

Hiswe commented Jul 31, 2014

If I understand what you're trying do to, I can make optional the <title> generation.
For the other optimisations it should be made with SVGO, no?

Although for the CSS generation, I'm thinking of implementing the same solutions as the asset-manifest of the gulp-rev plugin.
This ticket has be reopen for that reason :)

kud commented Jul 31, 2014

Yeah title should be optional.

Plus, SVGO didn't work for me on svgsymbols. (tried with imagemin)

Owner

Hiswe commented Jul 31, 2014

Ok for the title options (see https://github.com/Hiswe/gulp-svg-symbols/issues/7)

this plugin use SVGO before combining them.
If you can provide a simple test file and the expected result, I will try to figure out what happen :)

kud commented Jul 31, 2014

Yes sure, thank you ;)

Owner

Hiswe commented Aug 1, 2014

For the title, can you test with the branch accessibility ?

npm install git://github.com/Hiswe/gulp-svg-symbols.git#accessibility --save-dev

you can set the option accessibility to false to remove the tag of the SVG.

Owner

Hiswe commented Aug 5, 2014

It's now ok on the 0.1.5 version.
Tell me if there is some other issues with SVGO

kud commented Aug 19, 2014

Okay I'll check it if it works well, thanks ;)

Owner

Hiswe commented Sep 26, 2014

Ok Now I see what you really want to do :)
We can :

  • remove or merge all <defs> in the same place
  • remove or merge every <styles> (Yes I have encounter this issue quite recently)
  • I'm mot enough used to Sketch, so I don't know what kind of useless thing it puts in the SVG…

I have to dig in grunt-svgstore to see what kind of problems/optimisations they have fixed/done.

kud commented Sep 26, 2014

:D

VinSpee commented Oct 29, 2014

I would like to urge you to remove SVGO from this plugin. SVGO has a lot of issues in their most recent release that breaks very foundational things. It would be better to allow people to use something like https://github.com/sindresorhus/gulp-imagemin if they like, or at least allow them the option to turn SVGO off.

Owner

Hiswe commented Oct 30, 2014

OK.
I will begin migration this week-end.
I will do this in an another branch so if you have time, you'll be able to test.

VinSpee commented Oct 30, 2014

@Hiswe I can definitely help testing

Owner

Hiswe commented Nov 1, 2014

@VinSpee you can play with that:

https://github.com/Hiswe/gulp-svg-symbols/tree/svgo-removal

It should do the work & fix @kud issues.

I'm not sure about the api & providing some presets but I still want this to be easy for common users.
I still need to:

  • update the test
  • handle defs
  • handle id

Also, I've seen that gulp-svg-store is doing the same thing as this plugin… don't know if I should maintain this one.

VinSpee commented Nov 1, 2014

working perfectly here. Please do let us know if you decide to stop maintaining this.

kud commented Nov 2, 2014

^^

Owner

Hiswe commented Sep 30, 2015

@kud I have removed all SVG optimisation from this plugin: this should be handled outside gulp-svg-symbols.
I've make some tests with gulp-cheerio on this repo it should handle what you were trying to do.

@VinSpee As mentioned above, I've removed SVGO and the optimisations that come along.
Plugin still maintained ;)

@Hiswe Hiswe closed this Sep 30, 2015

kud commented Sep 30, 2015

Thanks. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment