added rcs- prefix for all CSS classes using CSS-Modules #51
Conversation
| @@ -1,8 +1,8 @@ | |||
| import React from 'react' | |||
| import TestUtils from 'react-dom/test-utils' | |||
| import reactDOM from 'react-dom' | |||
| import CustomScroll from 'customScroll.js' | |||
| import '../main/cs.scss' | |||
| import CustomScroll from '../main/customScroll.js' | |||
There was a problem hiding this comment.
I'm not sure why CustomScroll was imported from production instead of the dev environment.
There was a problem hiding this comment.
Thanks for contributing!
I can't see changes in the cs.scss file. Did you change the target css directly ?
Can you please explain the solution in general. I understand why I should add "rcs" prefix to all classes but what else does this change include ?
thanks
webpack.config.js
Outdated
| @@ -24,7 +24,7 @@ module.exports = [{ | |||
| test: /\.scss$/, | |||
| use: ExtractTextPlugin.extract({ | |||
| fallback: 'style-loader', | |||
| use: ['css-loader', 'sass-loader'] | |||
| use: ['css-loader?sourceMap&modules&camelCase=dashes&localIdentName=rcs-[local]', 'sass-loader'] | |||
There was a problem hiding this comment.
Why is this change needed ? I know that if you want source maps for css you need to add options object with sourceMap: true, to both sass-loader and css-loader
There was a problem hiding this comment.
@rommguy I didn't change the css file directly, i configured webpack to do it via css-modules.
This is why i refactor the usage of the css clasees.
So this
import './path/to/file.css
// ...
className="some-class"
Refactored to this:
import styles from './path/to/file.css
// ...
className={styles.someClass}
each class within the css file will be prefixed with the localIdentName (rcs in our case).
As for the sourceMap this was not intentional, I'm used to add this by default but it shouldn't be part of this PR. 😄
There was a problem hiding this comment.
Very cool, just read about the localIdentName, I wasn't familiar with that option, I thought it's always a generated hash.
Why did you decide to use the url format configuration instead of the options object ?
I saw the usage of options object here - css-loader
There was a problem hiding this comment.
Very cool, just read about the localIdentName, I wasn't familiar with that option
Yeah! i usually add the folder as well so i get the component name (depends on the files structure) as part of the css class.
class="MyComponent-my-class"
You can read about it here
Why did you decide to use the url format configuration instead of the options object ?
Actually no special reason, i usually use the options object, but i wasn't sure what format you like and it is inside the ExtractTextPlugin.extract object so i thought separating each loader to on object instead of string can look less readable.
If you think we need to go with options fine by me!
There was a problem hiding this comment.
ok, lets go for the options object, I prefer it over the url format
|
I merged, will publish a new version to npm. |
|
I dont know actually. |
should cover #43
I know there's another PR for adding a prefix for the css classes #45 but i think this solution using css-modules can perform better at scale.
Note that i updated the readme with the new
rcs-prefix and i think we should bump a major version as this is a breaking change.