thesabbir / simple-line-icons

Simple and Minimal Line Icons
https://thesabbir.github.io/simple-line-icons
MIT License
1.86k stars 846 forks source link

[FIX]: Fixing support for overriding the SCSS variables #117

Closed jrubins closed 4 years ago

jrubins commented 4 years ago

Context

I was updating our project's simple-line-icons dependency to the v2.5.2 version and when I went to test our build, I noticed that it was failing because our overriding of the $simple-line-font-path variable wasn't being applied.

When I looked at the changes made recently, I saw that the !default declarations had been dropped from the built SCSS file. In other words, older versions (specifically version v2.4.1) had the following:

$simple-line-font-path: "../fonts/" !default;

But in v2.5.2 that line looks like:

$simple-line-font-path: "../fonts/";

(See the full comparison to see these changes.)

Changes

Looking at how the SCSS file was being generated, I saw that the less2sass.js script was used for this transformation. There doesn't seem to be support there for default variables because LESS doesn't have a specific default declaration and instead just relies on ordering. Therefore, I didn't see anything in the less2sass code that would support adding this default declaration easily (because there's no good way to determine what should be a default variable vs what shouldn't be).

So the changes I made in this PR are to modify the less2sass.js script to support adding the !default declaration for the variables that we want to be overridable.

Testing

I used my forked repository in our project and it seemed to resolve the issue (i.e. the variables were overridable again).

garbinmarcelo commented 4 years ago

I opened an issue with this problem and ended up generating a pull request without seeing yours. I believe that the author can close mine in this case. Thank you

thesabbir commented 4 years ago

Please resolve conflict and do a build. I'll merge this.

thesabbir commented 4 years ago

@jrubins thanks for this pr 👍