Options to output width/height instead of background-size in SCSS mixin

[xposedbones]

In multiple scenario you need to enter the width/height of the svg instead of a background-size. I added an option to the mixin to output the width/height OR the background-size depending of the flag $use-background-size.

.hello {
    @include sprite('test', (), true, false);
    content: '';
}

Will output

.hello {
    content: '';
    background-image: (data:image...);
    width: 15px;
    height: 15px;
}

[cascornelissen]

Interesting change, I like it! 🤔

One thing I'm hesitant about is the fact that $use-background-size now requires $include-size to be true, e.g. @include sprite('test', (), false, false); results in the same as @include sprite('test', (), false, true); which is a bit of a code smell.

It's been a while since I've dabbed in Sass mixins, would it be possible to instead add a third (and maybe fourth) option to $include-size? We could do the following:

1. If $include-size equals false: don't do anything (like the current implementation)
2. If $include-size equals true: use background-size (like the current implementation)
3. If $include-size equals 'width-height': use width + height, maybe we should come up with a better string value like block or content-area...
4. If $include-size equals 'background-size': use width + height, optionally

[xposedbones]

In order to not break existing code, I think having @include-size equals width-height would be the best options. I'll commit that tomorrow

[xposedbones]

@cascornelissen Pushed! I think $include-size: 'box' makes the most sense here but I'm super open to use another keyword :)

[cascornelissen]

Nice, LGTM. Would you mind adding a test and updating the docs?

[xposedbones]

Nice, LGTM. Would you mind adding a test and updating the docs?

Sorry for the delay, personal stuff came up.

I can update the docs but I'm not sure how I'd write a test for this :/

[xposedbones]

@cascornelissen I'm looking at the docs and would write it in docs/variables.md but I'm not sure it belongs there. Should we create a new documentation for the sass mixin?

I first learned about the third argument by looking directly at the mixin because I was curious how it worked under the hood, there was no documentation about the third argument.

[cascornelissen]

Interesting, looks like we could use some docs improvements at the end of the document explaining what the Sass mixin is about I guess, I could also include that after merging this PR.

With regards to the tests, I'm guessing (hoping actually) that some tests are currently failing because they test equality with files in this directory. Updating these files and making sure all tests are green should be enough in this case. You can run tests with npm run test.

[xposedbones]

Wow that fell through the cracks haha.

I updated the tests and not a single test fails now :)

[cascornelissen]

No problem at all, if you could fix my last comment I'll make sure this gets merged and released as soon 3.6.x (Webpack 5 support) is finished.

[cascornelissen]

This feature was included in release 3.7.0, thanks for your contribution! 🚀