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

@import _variables.scss to each SASS file in project #1407

Closed
wants to merge 4 commits into from
Closed

@import _variables.scss to each SASS file in project #1407

wants to merge 4 commits into from

Conversation

gilhanan
Copy link
Contributor

  • What kind of change does this PR introduce?
    Feature:
    After using SASS in project, it's seem very essential to sharing _variables.scss in all SASS files, instead manually import it each time, which can be over head especially when styling Angular Components thats may locating deep in the project directories so the relative path to _variables.scss can be very long. (from experience)

  • Other information:
    Using sass-resources-loader

@gilhanan gilhanan changed the title @import _variables.scss to each SASS module in project @import _variables.scss to each SASS file in project Jan 23, 2017
@shlomiassaf
Copy link
Contributor

shlomiassaf commented Jan 23, 2017

@gilhanan I'm using sass resource loader, it's great.

The problem is that it does not play nicely with relative imports.
It requires you to add ALL virtual SCSS files into the resource array in the webpack plugin configuration which is something that adds a lot of noise to the starter.
I can live with that... but most devs won't know about it so they will just add @imports to the _variable file... we'll end up supporting sass resource loader.

About the commit

loader: `css-loader!sass-loader!sass-resources-loader?resources=${helpers.root('src/styles/_variables.scss')}```

This is getting too lengthy, we need to move it to an object based loader configuration.
It adds to my comment above, consider the need for multiple SCSS variable files...

This is a common thing, you usually divide these files to categories.

@shlomiassaf
Copy link
Contributor

BTW, i'm up supporting sass-resource-loader since in practice you can't work without it on a large project.

Maybe we should wait a bit for them to go out of beta...

Here is an example to a configuration using object

loader: ExtractTextPlugin.extract({
              fallbackLoader: 'style-loader',
              loader:  [
                'css-loader',
                'postcss-loader',
                'sass-loader',
                {
                  loader: 'sass-resources-loader',
                  query: {
                    resources: [
                       helpers.root('src/styles/_variables.scss')
                   ]
                  }
                }
              ]
            }),

I'll also point out that you can use imports from a main variable file, they just have to be absolute.

In my setup it:

  resources: [
                       helpers.root('src/styles/variables/_index.scss')
                   ]

But we can't put that on the starter, we'll end up confusing people.

@colinskow
Copy link
Contributor

Not everyone is going to want to import the same _variables.scss into all stylesheets. This isn't a convention I feel we should force on users.

@gilhanan
Copy link
Contributor Author

gilhanan commented Jan 25, 2017

@shlomiassaf Thanks for the feedback, I made another commit for changing the loader property to object based configuration.

About the PR, the meaning of _variables.scss is to declaring variables, so it don't have side effect and users can override variables which declared before in theirs styles, so it's just advantage. And to prevent confusing I think a good comment will clarify it.

@PatrickJS PatrickJS closed this Sep 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants