cascornelissen / svg-spritemap-webpack-plugin

SVG spritemap plugin for webpack
MIT License
210 stars 51 forks source link

Offer option to choose between inline svg url and external file for styles (SVG fragments) #36

Closed janwidmer closed 5 years ago

janwidmer commented 6 years ago

If using the styles option of your plugin, the generated css contains the icons es embedded data in the background url. I assume it is like this, because safari did not support css svg fragment identifiers until now. (https://css-tricks.com/svg-fragment-identifiers-work/#article-header-id-3) That has the downside, that if I use the same icon on 5 places in my code, the data for the icon is embedded 5 times. Of course I can try to use a post css tool to clean it up..

The recent safari update (safari 11.1 / safari IOS 11.3) added support for those fragement identifiers, see https://caniuse.com/#feat=svg-fragment

Therefore, it would be nice, if there would be an option to choose, if the css should contain the icons embedded in the url or just the url to the file, as it is for the html way of using it.

cascornelissen commented 6 years ago

The reason for this is indeed support in older browsers (which still have quite a big market shared). You're right about the downside but the styles options is only provided as a way to use the sprites when you really can't add them in your HTML (for whatever reason), so using the same icon more than 2 times sounds like another problem in itself.

That being said, we might be able to add an option for users to specify which type of styles inline SVG output they want. Something that could be integrated with something like browserlist as well. This could land simultaneously with changes proposed in https://github.com/cascornelissen/svg-spritemap-webpack-plugin/issues/35#issuecomment-393495046.

You're free to submit a PR, I'll have some time to look into this in the next few weeks as well.

janwidmer commented 6 years ago

yeah, I know that it's more for special cases like icons in richtext where you cannot control the markup and therefore it will not be such a big problem..offering such an option depending on browserlist would be quite nice and future ready..

I am quite busy right now with our project, so I will probably not have time right now. :-(

cascornelissen commented 6 years ago

@janwidmer, I'm hoping to include this in the upcoming 3.0.0 release. As far as I'm concerned two options need to be added (note that the options object will be refactored in this release as well):

  1. A new sprite.generate.view option (default: false) will be added which will generate the <view> elements with the correct viewBox attribute when enabled.
  2. A new styles.format option (default: 'data', can also be 'fragment') which will change the SVG code that gets generated for the styles file.

Any thoughts on this?

janwidmer commented 6 years ago

@cascornelissen sounds good..I guess, as a consequence, there will be two different sprites svg files, one for html usage and one for css usage? Or will it be possible to use the same sprites.svg file because the current viewBox parameter within the sprites.svg for html usage always has 0 for left / top parameters: viewBox="0 0 20 20"?

cascornelissen commented 6 years ago

The goal is to generate a single SVG file.

The current viewBox you're talking about is the attribute on the root <svg> element, right? This new approach would generate <view> elements which have their own viewBox attribute matching the x/y/width/height of the sprite.

janwidmer commented 6 years ago

that would be great. I am talking about the viewbox param, which every symbol in the generated svg file has:

<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="238" height="1093.8">
    <symbol id="action-add" viewBox="0 0 20 20">
        <style>.st0{fill:none}</style>
        <path d="M18 9.2h-7.2V2H9.2v7.2H2v1.6h7.2V18h1.6v-7.2H18z" id="icons"/>
    </symbol>
    <use xlink:href="#action-add" width="20" height="20"/>
    <symbol id="action-delete" viewBox="0 0 20 20">
        <style>.st0{fill:none}</style>
        <path d="M3 9.2h14v1.5H3z" id="temp"/>
    </symbol>
    <use xlink:href="#action-delete" y="22" width="20" height="20"/>
    <symbol id="action-edit" viewBox="0 0 20 20">
        <style>.st0{fill:none}</style>
        <path d="M14.7 3.1l2.2 2.2L5.7 16.5H3.5v-2.2L14.7 3.1m0-2.1L2 13.6V18h4.4L19 5.3 14.7 1z" id="icons"/>
    </symbol>
</svg>

And if for the css usage a different viewbox is needed (Having the correct top / left value set), it might not work anymore for the html way?

cascornelissen commented 6 years ago

Ah, I think we're talking about different things here:

  1. If you want to use sprites in CSS the old way (the entire SVG string in the CSS file) you'll have to enable the sprite.generate.use option which will generate the <use> elements shown in your codeblock, this is also available in the current 2.x version of the plug-in.
  2. If you want to use sprites like you suggested in this issue (most of the SVG in a separate file, with a URL + fragment identifier, e.g. background: url('/spritemap.svg#action-edit') no-repeat;) you'll have to enable the sprite.generate.view option which will generate the <view> elements like so:

    <view id="action-edit" viewBox="0 0 20 20"/>

I think they should be able to coexist so I might add a use- and view- prefix (configurable) to the id attributes as they're supposed to be unique.

Is the SVG you posted in the previous comment the output of the plug-in? That would mean something indeed goes wrong with the use's viewBox attribute. If that's actually generated code, could you create a separate issue for it and I'll make sure to fix it in the 3.x release as well.

janwidmer commented 6 years ago

prefixing the id's with use- or view- would make sense. The above output is actual code generated from the plugin, see attached file. But I am not sure, what is actually wrong, it seems to be working..?! sprites.svg.zip

cascornelissen commented 6 years ago

~It should work no matter the actual value of the viewBox, it would just be nicer to actually show the correct values but I'll have to dig into it a little further. Thanks for the attachment!~

Scrap that, see https://github.com/cascornelissen/svg-spritemap-webpack-plugin/issues/50#issuecomment-425429228.

janwidmer commented 6 years ago

ah ok..would the correct viewbox value have top / left values then? updated the zip again and removed two icons which are client specific, please use the new one

janwidmer commented 6 years ago

Issue: https://github.com/cascornelissen/svg-spritemap-webpack-plugin/issues/50

cascornelissen commented 6 years ago

@janwidmer, this has been added in the 3.x release which is currently a Release Candidate, you can install this version using:

npm install svg-spritemap-webpack-plugin@next

The original issue you reported should be fixable by using the following options, the rest of your configuration might need some restructuring for this version as well so be sure to check the new documentation on options.

new SVGSpritemapPlugin({
    sprite: {
        generate: {
            use: true,
            view: '-view'
        }
    },
    styles: {
        format: 'fragment'
    }
});

Is this something you could check? If so, I'd love to hear the results so I can finish up the upcoming major release.

janwidmer commented 6 years ago

@cascornelissen cool thanks, we will try it out in the next few days and give you feedback.

janwidmer commented 6 years ago

@cascornelissen I just tried 3.0.0-rc.2 with the following config:

new SVGSpritemapPlugin(path.join(appDirectory, svgSpritesSrc), {
    output: {

        filename: path.join(svgSpritesPublicPath, 'sprites.svg'),
        svg4everybody: true,
    },
    sprite: {
        prefix: false,
        generate: {
            use: true, // generates use tags within the svg to use in css via base 64 data url
            view: '-view', // generate view tags within the svg to use in css via fragment identifier url and add -view suffix for fragment id
            symbol: true, // generate symbol tags within the svg to use in html via use tag
        },
    },
    styles: {
        format: 'fragment', // determines which url should be written to the scss sprite map
        filename: '~sprites.scss',
    },
})

That generates the attached file: sprites.svg.zip which seems to be correct from what I can see.

The "Use" Option works as it did before, the "View" Option with the new fragment solution renders an empty background url background: url() center no-repeat; in my css.

Do you see any obvious mistakes in my config? Using data as styles.format value also gives me an empty background url, so I am probably doing something wrong..

cascornelissen commented 6 years ago

Not sure and not in a position to check right now but I think if you add sprite.generate.use with value true it should work correctly? It should also show a warning in the Webpack output about this configuration mismatch as well.

janwidmer commented 6 years ago

there is no warning in the console, so I assume the config is correct then..background url is still empty.. i will investigate further..if you have inputs at some point (not right now ;-)) that would be great..

cascornelissen commented 6 years ago

You're right about there not being a warning, the default value for sprite.generate.use is still true in 3.0.0-rc.2 ~(which will change to false in 3.0.0-rc.3)~.

The SVG you've attached seems to be correct as well and I just tried replacing the SCSS example with the configuration you've provided (I removed the output.filename key and replace the styles.filename value with the one that's in the example already) and I get the correct values in the output _sprites.scss and compiled CSS bundle.

I'm at a loss about what problem you're running into, a simple reproducible case (just a package.json, webpack.config.js and the required assets) would probably help. Wondering if you're able to find anything!

janwidmer commented 6 years ago

it seems, that something with the parameter sprite.generate.view might be wrong. I am setting this parameter to -view.

That correctly generates the svg like <view id="action-add-view" viewBox="0 0 21 21"/> but in the generated scss map, I have the following entry 'action-add': "/etc/designs/slevo/clientlibs/assets/sprites.svg#action-add", which seems to miss the suffix part from the config..

Did you try it also with a suffix?

janwidmer commented 6 years ago

Something with the prefixes is definitely weird or I am understanding it wrong.

Case 1: Config:

sprite: {
    generate: {
        symbol: true,
        use: true,
        view: '-view'
    },
},

If I understood it correctly, the above config should both generate symbol tags, use tags and view tags and additionally, the view tag should have the suffix -view to be used within css by fragment identifier?

The generated scss sprite map contains 'action-add': "/sprites.svg#action-add",

The generated sprites.svg contains:

<symbol id="action-add" viewBox="0 0 20 20">
    <style>.st0{display:none}.st13{fill:none}.st16{fill:#231f20}</style>
        <path
            d="M18 9.8c0-.3-.2-.5-.5-.5h-6.8V2.5c0-.3-.2-.5-.5-.5h-.4c-.3 0-.6.2-.6.5v6.8H2.5c-.3-.1-.5.2-.5.5v.5c0 .3.2.5.5.5h6.8v6.8c0 .3.2.5.5.5h.5c.3 0 .5-.2.5-.5v-6.8h6.8c.3 0 .5-.2.5-.5v-.5z"
            id="icons"/>
</symbol>
<use xlink:href="#action-add" width="20" height="20"/>
<view id="action-add-view" viewBox="0 0 21 21"/>

Case 2: Config:

sprite: {
    generate: {
        symbol: '-symbol'
        use: true,
        view: true
    },
},

If I understood it correctly, the above config should both generate symbol tags, use tags and view tags and additionally, the symbols should get the prefix -symbol. and the icon can be referenced in html by `?

The generated scss sprite map contains 'action-add-symbol': "/etc/designs/slevo/clientlibs/assets/sprites.svg#action-add-symbol",

The generated sprites.svg contains:

<symbol id="action-add-symbol" viewBox="0 0 20 20">
    <style>.st0{display:none}.st13{fill:none}.st16{fill:#231f20}</style>
    <path
            d="M18 9.8c0-.3-.2-.5-.5-.5h-6.8V2.5c0-.3-.2-.5-.5-.5h-.4c-.3 0-.6.2-.6.5v6.8H2.5c-.3-.1-.5.2-.5.5v.5c0 .3.2.5.5.5h6.8v6.8c0 .3.2.5.5.5h.5c.3 0 .5-.2.5-.5v-6.8h6.8c.3 0 .5-.2.5-.5v-.5z"
            id="icons"/>
</symbol>
<use xlink:href="#action-add-symbol" width="20" height="20"/>
<view id="action-add" viewBox="0 0 21 21"/>

Shouldn't in Case 2 the scss sprite map NOT contain any suffixes, but only in Case 1, because there I specify a suffix within the viewparameter?

cascornelissen commented 6 years ago

@janwidmer, thanks for the detailed report. Everything you said is correct and I'll test these use cases specifically tonight, seems like something is wrong in the postfix like you noticed.

Just to be sure, the config excerpts you posted in your comment, these these also included the styles.format: 'fragment' option, right?

janwidmer commented 6 years ago

yep they do, in my above comment you can see the full config (https://github.com/cascornelissen/svg-spritemap-webpack-plugin/issues/36#issuecomment-427375008)

cascornelissen commented 6 years ago

@janwidmer, got the time to look into this and I think I've fixed it in master, could you install directly from GitHub and check again?

npm install cascornelissen/svg-spritemap-webpack-plugin#master

Thanks in advance!

janwidmer commented 6 years ago

cool thanks I will try it on monday

janwidmer commented 6 years ago

@cascornelissen just tried it out with the current version from github. The ID generation now works correctly and I get to see some svg content when I apply it via css. But there seems to be something wrong with the generated numbers of the viewbox:

Here is the svg content for the checkbox icon:

<symbol id="checkbox" viewBox="0 0 14 14">
    <style>.st0{fill:#fff}</style>
    <path
        d="M11.6 3.1c-.2-.2-.5-.2-.7 0L5.6 8.4 3.1 5.9c-.2-.2-.5-.2-.7 0l-.7.7c-.2.2-.2.5 0 .7l2.1 2.1c.2.3.5.6.7.8l.7.7c.2.2.5.2.7 0l.7-.7c.2-.2.6-.5.8-.7l5-5c.2-.2.2-.5 0-.7l-.8-.7z"
        id="Icons"/>
</symbol>
<use xlink:href="#checkbox" y="168" width="14" height="14"/>
<view id="checkbox-view" viewBox="0 167 15 183"/>

The generated viewbox should be something like this in my opinion: <view id="checkbox-view" viewBox="0 172 14 14"/> (numbers based on my manual calculation of the icons coming before the checkbox in the sprites.svg)

any idea? sprites.svg.zip

cascornelissen commented 6 years ago

@janwidmer, the offsets being off by 1px are probably caused by the fact that sprite.gutter defaults to 2 and these are included in the calculations.

That would probably also explain the offset in the use tag, although I can't find a symbol with id checkbox in the zip you've attached.

Could you try setting the sprite.gutter option to 0 to check if it calculates correctly then? I'm thinking about changing this default value to 0 anyway.

janwidmer commented 6 years ago

@cascornelissen setting the gutter to 0 removes the "weird" offset, but the viewbox of the checkbox icon is still wrong: <view id="checkbox-view" viewBox="0 152 14 166"/>. The fourth value should be also 14px (height of the icon) and not the addition of the Y value and the icon height..or am I wrong? (See this article https://css-tricks.com/svg-fragment-identifiers-work/)

You can see the symbol with the id checkbox on line 68 of the above sprites.svg file..

cascornelissen commented 6 years ago

Ahh, I finally get what you mean and you're right, sorry for the confusion. Will fix this ASAP!

cascornelissen commented 6 years ago

@janwidmer, finally found some time to dig into this and it should be fixed in the 3.0.0-rc.3 release. Could you check it out and report back?

cascornelissen commented 5 years ago

ping @janwidmer

janwidmer commented 5 years ago

Hey @cascornelissen , Sorry, was also busy, I will try to test it this week

cascornelissen commented 5 years ago

No problem at all, I'm hoping to release 3.0.0 tomorrow (friday) morning (CET). If you aren't able to check it before then I'll make sure to test it against the latest master commit just to be sure.

janwidmer commented 5 years ago

ok il try to test it today then

janwidmer commented 5 years ago

@cascornelissen perfect, now it works. thanks for the quick support.

cascornelissen commented 5 years ago

Awesome, thanks for the update. Any chance you can verify the fix for #35 as well?