search

Lemoncode / LeanMood

MIT License
27 stars 8 forks source link

Choose a styles architecture approach (SASS, BEM, CSS Modules ? ) #46

Closed brauliodiez closed 7 years ago

brauliodiez commented 7 years ago

As @franmolmedo mentioned, there is an open area here, we need to discuss and choose which approach to follow when handling styles:

@red your input is really appreciated here as well.

franmolmedo commented 7 years ago

My two cents: I would choose CSS-Modules.

brauliodiez commented 7 years ago

Thanks for the feedback @franmolmedo , the approach could be?

Do you agree?

franmolmedo commented 7 years ago

That´s right. We need to update webpack configuration adding a new loader that makes the trick. Besides that, developers don´t have to be worried about naming and collisions because this loader assings an unique identifier to each component and then prepends this identifier to all styles related.

brauliodiez commented 7 years ago

Is this loader a good one to give a try?

https://github.com/gajus/react-css-modules

Not sure if maybe @linuxonrails would like to give a try on this case :).

franmolmedo commented 7 years ago

The common css loaders also works https://github.com/webpack/css-loader, but the configuration changes a bit, something like: test: /.(scss|sass)$/, loaders: ['style', 'css?modules&importLoaders=1', 'postcss', 'sass'],

Also it has to be updated in test configuration if we want to test if certain styles are applied to a component in concrete circunstances.

brauliodiez commented 7 years ago

That's true, we have to evaluate which o both fits best our needs.

linuxonrails commented 7 years ago

An example with css-modules (I had some issues with typescript):

https://github.com/linuxonrails/react-by-sample/tree/cssmodules/16%20CSSModules

brauliodiez commented 7 years ago

Cool stuff !!! @nasdan @franolmedo @jaimesalas what do you think?

What about having a common SASs file with the theming colores?

JaimeSalas commented 7 years ago

On my humble opinion, I think right now that is our best choice, since I discuss with @brauliodiez the issues that comes with CSS Module. Looks like loads of fun ;)

linuxonrails commented 7 years ago

CSS Modules has too many problems with TypeScript:

You need generate typings of each css file (very slow). And reopen Atom editor because it doesn't refresh typings when the file is open in a tab.

If you have an error in stylesheet file the typings file is not generated (but you will not know why: no console errors).

Example A:

.table {
  composes: table from './table.css';
}

.row {
  composes: row from './table.css';
}

.table {
  width: 400px;
}

Problem? The .table selector is repeated.

Example B:

.table-foobar {
  color: red;
}

Problem? table-foobar is not a valid variable name.

Example C:

.table {
  composes: table from './fileNotExists.css';
}

Problem? Yes. No errors in atom but... typings will not be generated. Another silent error. https://github.com/gajus/react-css-modules#extending-component-styles

And...

another problem in JSX code!!

import * as React from 'react';
import CSSModules from 'react-css-modules';
import * as styles from './foobar.css'; // .foobar { color: red }

class Foobar extends React.Component<{}, {}> {
  render () {
    return <div styleName='foobar'>
      This text should be red!
    </div>;
  }
}

export default CSSModules(Foobar, styles);

react-css-modules require overwrite render() method. export { CSSModules(Foobar, styles) } doesn't work m-( It works only with default export. And I think it is ugly to add that code in each file.

...but we can use ES7 decorators:

import * as React from 'react';
import CSSModules from 'react-css-modules';
import * as styles from './foobar.css'; // .foobar { color: red }

@CSSModules(styles)
export class Foobar extends React.Component<{}, {}> {
  render () {
    return <div styleName='foobar'>
      This text should be red!
    </div>;
  }
}

...but TypeScript has an experimental support for decorators:

(6,14): error TS1219: Experimental support for decorators is a feature that is subject to change in a future release. Set the 'experimentalDecorators' option to remove this warning.

I will continue investigating...

linuxonrails commented 7 years ago

lol

<--- Last few GCs --->

[9846:0xa7cfb80] 546246 ms: Mark-sweep 700.6 (803.2) -> 700.6 (803.2) MB, 3165.3 / 0.0 ms (+ 1.3 ms in 3 steps since start of marking, biggest step 1.3 ms) allocation failure GC in old space requested [9846:0xa7cfb80] 549173 ms: Mark-sweep 700.6 (803.2) -> 700.6 (802.2) MB, 2925.8 / 0.0 ms (+ 1.3 ms in 1 steps since start of marking, biggest step 1.3 ms) last resort gc [9846:0xa7cfb80] 551798 ms: Mark-sweep 700.6 (802.2) -> 700.6 (802.2) MB, 2624.4 / 0.0 ms last resort gc

<--- JS stacktrace --->

==== JS stack trace =========================================

Security context: 0x2f9739dd 1: / anonymous /(aka / anonymous /) [/home/davidm/src/linuxonrails/react-by-sample/16 CSSModules/node_modules/typed-css-modules/lib/fileSystemLoader.js:88] [pc=0x24c58edd](this=0x2f9081a1 ,_ref=0x24af0465 <an Object with map 0x3af33189>) 2: PromiseHandle(aka PromiseHandle) [native promise.js:~94] [pc=0x3c0f596e](this=0x2f9081a1 ,A=0x24af0465 <an Object with map 0x3...

FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - JavaScript heap out of memory 1: node::Abort() [node] 2: 0x8e56266 [node] 3: v8::Utils::ReportOOMFailure(char const, bool) [node] 4: v8::internal::V8::FatalProcessOutOfMemory(char const, bool) [node] 5: v8::internal::Factory::NewRawOneByteString(int, v8::internal::PretenureFlag) [node] 6: v8::internal::String::SlowFlatten(v8::internal::Handle, v8::internal::PretenureFlag) [node] 7: v8::internal::String::Flatten(v8::internal::Handle, v8::internal::PretenureFlag) [node] 8: v8::internal::StringTable::LookupString(v8::internal::Isolate, v8::internal::Handle) [node] 9: v8::internal::LookupIterator::PropertyOrElement(v8::internal::Isolate, v8::internal::Handle, v8::internal::Handle, bool*, v8::internal::LookupIterator::Configuration) [node] 10: v8::internal::Runtime_SetProperty(int, v8::internal::Object*, v8::internal::Isolate) [node] 11: 0x2c50a23e 12: 0x24c58edd 13: 0x3c0f596e

brauliodiez commented 7 years ago

Have you tried using requiere instead of import?

Maybe we loose some intellisense.but it.could work

linuxonrails commented 7 years ago

Thanks @brauliodiez !

Another option to generate typings on-the-fly: https://github.com/Jimdo/typings-for-css-modules-loader

linuxonrails commented 7 years ago

It's works with require in a test case:

import * as React from 'react';
const css = require('react-css-modules');
const styles = require('./table.css');

@css(styles)
class Table extends React.Component<{}, {}> {
  render() {
    return (
      <div styleName='foobar'>
        This should be in red color!
      </div>
    );
  }
}

export { Table };

But the next line in tsconfig.json:

  "types": ["karma-chai-sinon"]

throws an error in TS:

ERROR in ./table.tsx (9,12): error TS2339: Property 'styleName' does not exist on type 'HTMLProps'.

linuxonrails commented 7 years ago

I will work in other issues the next days :-(

brauliodiez commented 7 years ago

No prob, I will take a look, we will end up with some stuff :).

Give me a buzz if you run out of issues, I have some more pending to define :).

brauliodiez commented 7 years ago

Good work, going to reassign this issue to @Nasdan he will do some further research. He will buzz us whenever there is any update.

nasdan commented 7 years ago

I've create an issue #57 to implement it.

brauliodiez commented 7 years ago

A quick heads up, CSS Modules does not support nested selectors:

https://github.com/css-modules/css-modules/issues/131

Since we are developers we can live without this, but if we ask a designer to work on this probably he will start complaining about this.

Inside the same css it will work fine, the issue come, when, for instance you want style children elements that are below a bootstrap 'well'.

What's your take on this? @Nasdan @franmolmedo @linuxonrails

brauliodiez commented 7 years ago

We are evaluating TypeStyle:

https://github.com/typestyle/typestyle

Const: going to full typescript way.

Pros: Support all the CSS stuff + you get intellisense + js support + lazy loading

Gray areas: Now it generates random unique names, we have to check whether there is a way to add basename + prefix?

nasdan commented 7 years ago

Failing case:

styles.css

.pageError404 {
  margin-top: 6em;
}

.pageError404 .panel {
  background-color: blue;
}

page.tsx

import * as React from 'react';
import {pageError404} from './styles.css';

export const NotFoundPage = () => {
       return (
         <div className={`row ${pageError404}`}>
           <div className="col-md-8 col-md-offset-2">
             <div className="panel panel-danger">
             </div>
           </div>
         </div>
       );
}
brauliodiez commented 7 years ago

About SASS import duplicate (first let's check if sass-loader in fact generate duplicates or not), it seems that there are workarounds available (node-sass-import-once).

https://github.com/at-import/node-sass-import-once

http://stackoverflow.com/questions/36926521/webpack-sass-loader-cannot-dedupe-common-scss-imports

http://stackoverflow.com/questions/36339163/how-to-import-a-global-scss-file-with-webpack-only-once-and-not-in-every-singl

franmolmedo commented 7 years ago

I can speak only if working with ES6: This way the example works:

master.sass

@import "../../content/variables.sass"

.main
    border: 2px solid $master-border-color
    color: $basic-font-color
    .test
      background-color: red
      color: green
      &:hover
        background-color:blue !important
        cursor: pointer`

master.js

import React, { Component } from 'react';

import masterStyles from './master.sass';

class Master extends Component {
  render() {
    return (
      <div className={ masterStyles.main }>
         Master page
        <div className={ masterStyles.test }> HELLO </div>
      </div>
    );
  }
}

export { Master };

On the other way, we can check if any postcss action can satisfy all our needs.

And about double imports: no content duplicated in the bundled styles.css at least using variables.sass as global variables resources.

JaimeSalas commented 7 years ago

It is not about duplicate, it is about that you brought all the file (if you import varibles.sass, you bring all the variable), instead you have a .ts file, you can import just the staff that you want. That is the key difference. I do not really know, if there is a way to do that wit sass, but feels natural to be done with TypeStyle

franmolmedo commented 7 years ago

I don´t get it. As far i know imports in sass or less are everything or nothing. Another idea is not using a big global variables files and use small files to import in. Using typestyle is another option. I haven´t yet used it in a real environment. No comments though

brauliodiez commented 7 years ago

@franmolmedo is right, my fault, dummy mistake, no matter how many times your import your SASS it will get converted just to CSS. I think right now we should go for CSS Modules, and play in a lab with typescript css experiment. @nasdan can you setup this and ask for PR, we have a nice progress bar component made by @rumbado waiting for css modules love :).

nasdan commented 7 years ago

There is a PR #67 with all SASS and CSS Modules stuff

brauliodiez commented 7 years ago

Closing thia issue, CSS modules approach os on máster, se Will hace some fin un the future when creating component libraries but that's a.separate issue