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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Introduce CSS modules & covert /src/options over to using them #34

Merged
merged 1 commit into from Feb 28, 2017

Conversation

aquibm
Copy link
Collaborator

@aquibm aquibm commented Feb 27, 2017

Why?

This approach is much less restrictive than using the css-in-js approach that we had earlier. We're simply writing css here and can therefore easily style pseudo elements and make use of css animations. It also brings in new css features (variables, nesting, etc) and follows the golden rule that all styles are local to the component unless explicitly made global. (see routeTitle in base.css)

Changes:

  • The cssOutput prop in the bundle determines whether we'll attempt to build css-modules.
  • Stop tracking extension/options/style.css as this is now automatically generated via css-modulesify.
  • Covert over all the css-in-js styles in options to use css-modules.
  • Add autoprefixing and other features via cssnext

Future Tasks:

  • Convert the overview app to use css-modules

RFC?

This is the first time I've used css-modules through browserify, not sure if it's setup correctly. @Treora, please let me know if I need to change anything 馃憤

@aquibm aquibm requested a review from Treora February 27, 2017 08:14
@Treora
Copy link
Collaborator

Treora commented Feb 27, 2017

I am starting to like the idea of css-modules, to organise style in the same hierarchy and to make it traceable where a component's style is defined. It looks like css-next gives the main features I wanted to use scss for, but now with at least the pretension that it will be normally supported css some day.

If you like this approach I am happy to apply it to the whole project. In any case this seems better than either inline styles defined in javascript or having a single css file.


const Navigation = ({ currentLocation, routes }) => {
function buildRoutes() {
let cx = classNames.bind(styles)
Copy link
Collaborator

@Treora Treora Feb 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the css-modules docs about classNames.bind:

Note that in ES2015 environments, it may be better to use the "dynamic class names" approach documented above.

I agree with them, to avoid this extra level of magic.

let navClasses = cx({
navLink: true,
isActive: isActive(route)
})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so instead of cx this would be:

classNames({
    [styles.navLink]: true,
    [styles.isActive]: isActive(route),
})


class SettingsContainer extends React.Component {
render() {
return (
<div>
<h1 style={styles.title}>Settings</h1>
<h1 className="routeTitle">Settings</h1>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of making routeTitle a global class, would it not be cleaner to import { routeTitle } from '../../base.css'?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's much nicer, implemented. 馃憤

b.plugin('css-modulesify', {
output: path.join(destination, cssOutput),
postcssBefore: [
require('postcss-cssnext')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would import this right at the top rather than require it here. Also 'css-modulesify' could be imported, rather than passed as a string, for clarity and consistency.
Besides that this looks like a neat approach. Runs fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed 馃憤

@@ -35,15 +35,20 @@
"tldjs": "^1.7.0"
},
"devDependencies": {
"autoprefixer": "^6.7.5",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be an weird peer dependency of cssnext. Build fails without it.

package.json Outdated
"gulp": "^3.9.1",
"gulp-identity": "^0.1.0",
"gulp-uglify": "^2.0.1",
"loose-envify": "^1.3.0",
"postcss": "^5.2.15",
"postcss-cssnext": "^2.9.0",
"postcss-modules-values": "^1.2.2",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only postcss-cssnext is used directly, can the other two be removed without breaking it?

@@ -8,6 +8,9 @@ import watchify from 'watchify'
import babelify from 'babelify'
import envify from 'loose-envify/custom'
import uglifyify from 'uglifyify'
import path from 'path'
import modulesify from 'css-modulesify'
import cssnext from 'postcss-cssnext'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpicking: I'd keep the whole name and camelCase it as cssModulesify, especially since modulesify gives no hint what it does. Not sure between cssnext/postcssCssnext.

@Treora
Copy link
Collaborator

Treora commented Feb 28, 2017

Apart from these details, would this be ready to merge?

@Treora
Copy link
Collaborator

Treora commented Feb 28, 2017

By the way, feel free to squash all the tiny fixes in a single commit, keeping up the appearance that everything was perfect at the first shot. ;)

@aquibm
Copy link
Collaborator Author

aquibm commented Feb 28, 2017

Yep, this is good to merge :shipit:

- Stop tracking extension/options/style.css as this is now automatically generated via css-modulesify
- Covert over all the css-in-js styles in options to use css modules
@Treora Treora merged commit 48d2167 into WebMemex:master Feb 28, 2017
Treora added a commit that referenced this pull request Feb 28, 2017
[RFC] Introduce CSS modules & covert /src/options over to using them
Treora added a commit that referenced this pull request Feb 28, 2017
Introduce CSS modules & covert /src/options over to using them
@Treora Treora mentioned this pull request Mar 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.

None yet

2 participants