cascornelissen / svg-spritemap-webpack-plugin

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

Scss with format fragment and prefix function #151

Closed refsz closed 3 years ago

refsz commented 3 years ago

Hi, I use your plugin in almost every project and like it very much, thanks for that. Now I wanted to test in scss/css fragments, but unfortunately the prefix function is not executed there.

Expected behavior

$svg-sprites: (
    'sprite-client-general-check': '/build/base/spritemap.svg#sprite-client-general-check' 
)

Actual behavior

$svg-sprites: (
    'sprite-client-general-check': "/build/base/spritemap.svg#(sprite) => {
                let segments = sprite.split(path.sep);
                const index = segments.indexOf('assets')

                if (index) {
                    // removes all segments before theme name and the last
                    segments = segments.slice(index - 1, segments.length - 1);
                    // removes segments for asset and icons
                    segments.splice(1, 2);
                }

                return `sprite-${segments.join('-')}-`
            }sprite-client-general-check",

System information svg-spritemap-webpack-plugin: ^3.8.3 => 3.8.3

Minimal reproduction

path: 'htdocs/web/themes/**/assets/icons/**/*.svg',
config: {
        output: {
            filename: 'spritemap.svg',
            svgo: svgoConfig,
            chunk: {
                name: 'spritemap',
                keep: false
            },
        },
        sprite: {
            prefix: (sprite) => {
                let segments = sprite.split(path.sep);
                const index = segments.indexOf('assets')

                if (index) {
                    // removes all segments before theme name and the last
                    segments = segments.slice(index - 1, segments.length - 1);
                    // removes segments for asset and icons
                    segments.splice(1, 2);
                }

                return `sprite-${segments.join('-')}-`
            },
            idify: (filename) => filename.replace(/[\s]+/g, '-'),
            gutter: 0,
            generate: {
                title: true,
                symbol: true,
                use: true,
                view: true
            }
        },
        styles: {
            filename: 'svg-sprites.scss',
            format: 'fragment',
            keepAttributes: true,
            variables: {
                sprites: 'svg-sprites',
                sizes: 'svg-sizes',
                variables: 'svg-variables',
                mixin: 'sprite'
            }
        }
    }
cascornelissen commented 3 years ago

Thanks for submitting this interesting bug @refsz, it was a little hard to test properly but I think the changes in e3572b1 should solve your issue. Please test the changes out by installing the plugin directly from GitHub and letting me know your results.

$ npm install cascornelissen/svg-spritemap-webpack-plugin#master
refsz commented 3 years ago

Hi @cascornelissen , thanks for your quick reply. The fragment looks correct but the key has changed and is without prefix. I think it was with a prefix before that. Even if you configured data as the format.

$svg-sprites: (
    'check': "/build/svg/spritemap.svg#sprite-client-general-check"
)
cascornelissen commented 3 years ago

That's the way it was implemented initially, afterwards the ability to provide a callback function to the prefix option was added. Changing that now would mean a breaking change or yet another option that could toggle this functionality on (default would be off).

The only reason I can imagine where you'd want to have the prefix (and postfix) in the key is when there would/could be duplicates. In those cases using multiple instances of the plugin probably makes more sense.

If you're able to convince me/sketch a scenario where it makes more sense to have the prefix + postfix in the key than using multiple plugin instances in your webpack.config.js I can take a shot at adding the option but I want to ensure it's worth the effort.

refsz commented 3 years ago

I can only say that in the current version (3.8.3) the prefix is part of the key in the scss map. I don't know how far back this behaviour goes. In general i think the key in the scss should be identical to the symbol ID because with both you reference the sprite.

Yes, I use the prefix function to avoid duplicates. Basically I have two themes in the project (Base & Custom) and each theme has an icon folder, which in turn can have other subfolders. Depending on this, it may be that icons with the same file name are stored there.

You are of course right that I can avoid this with two instances of the plugin. But then I have to configure the options for the SCSS differently and use a mixin depending on the theme. But this way I don't escape the problem of duplicates in different subfolders.

For what reason was the callback for the prefix added?

cascornelissen commented 3 years ago

You are 100% correct, my brains are a bit hazy on the details but if it's like that in 3.8.3 we should keep it that way. If I find some time in the coming days this is on the top of my todo list.

For context, it was added in 3.3.0, the release notes link to the PR/issue so there's more information to find there.

cascornelissen commented 3 years ago

It took a bit of time and debugging (it's actually a bug/inconsistent behavior) but the changes in b3e2b24 introduce a new sprite.prefixStylesSelectors option (default false) which can be enabled to add prefixes to all styles-related selectors.

@refsz, if you have some time can you validate that this is working as intended?

$ npm install cascornelissen/svg-spritemap-webpack-plugin#master
refsz commented 3 years ago

The new option works with scss as intended. Thanks for the quick implementation.

cascornelissen commented 3 years ago

This change was included in 3.9.0 🚀