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

Scss with format fragment and prefix function #151

Closed
refsz opened this issue Feb 5, 2021 · 8 comments
Closed

Scss with format fragment and prefix function #151

refsz opened this issue Feb 5, 2021 · 8 comments
Assignees
Labels

Comments

@refsz
Copy link

refsz commented Feb 5, 2021

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'
            }
        }
    }
@refsz refsz added the bug label Feb 5, 2021
@cascornelissen
Copy link
Owner

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
Copy link
Author

refsz commented Feb 6, 2021

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
Copy link
Owner

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
Copy link
Author

refsz commented Feb 10, 2021

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
Copy link
Owner

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 cascornelissen self-assigned this Feb 11, 2021
@cascornelissen
Copy link
Owner

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
Copy link
Author

refsz commented Feb 16, 2021

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

@cascornelissen
Copy link
Owner

This change was included in 3.9.0 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants