Discussion: CSS methodologies (OOCSS/BEM/SMACSS/...) in creating/refactoring with Aphrodite

[luixxiul]

• Describe the issue you encountered: https://github.com/brave/browser-laptop/commit/234cfba499ababd498b73ca31065219f9e369d4c#commitcomment-21773452

Adding styles without semantic rules is a quite bad habit, which actually has resulted in creating CSS preprocessors like LESS and SCSS, and I notice that such a case started happenning on our project as well. We should consider the way of avoiding it somehow. I'm afraid of it that otherwise it would be very difficult to keep style consistency.

@cezaraugusto and @bsclifton agree with me that some kind of methodology to avoid styles from scattering and keep them consistent should be adopted, until Aphrodite will (maybe) start supporting nesting styles like we do with LESS. By creating components with one of the methodologies it will be easy to nest the code blocks later.

At first we need to discuss which methodology we are going to use. At a glance BEM will be suited for us. See: https://github.com/brave/browser-laptop/pull/7831/files#diff-fa59d1e0221a5bee94fd2abd9b96dcfdR158

[cezaraugusto]

I think BEM fill our needs nicely. Defining block/element/modifier for our codebase seems straightforward:

.site-search {} /* Block */
.site-search__field {} /* Element */
.site-search--full {} /* Modifier */

and

.person {}
.person__hand {}
.person--female {}
.person--female__hand {}
.person__hand--left {}

Seems easy to apply that pattern to a component such as buttons:

'browserButton': {}, // block
'browserButton__icon': {}, // element
'browserButton--primary': {} // modifier
'browserButton--primary__icon': {} // mixing it all

Looks good, improves readability and is easy enough to get started without digging too much on the methodology.

I found this article a great resource on how to get started: https://csswizardry.com/2013/01/mindbemding-getting-your-head-round-bem-syntax/

++ from me

[cezaraugusto]

I started to wonder if we could use a custom BEM style, so instead of using dashes we could use only one underline for modifiers? This way we could get ride of quotes. Sorry if bikeshedding just want to make sure we have a definitive guide for this.

browserButton: {}, // block
browserButton__icon: {}, // element
browserButton_primary: {} // modifier
browserButton_primary__icon: {} // mixing it all

[NejcZdovc]

@cezaraugusto yeah I like this one. I hate strings for properties

[cezaraugusto]

@luixxiul please ping us here if you have any objections or otherwise let's move forward with that pattern for after 0.15.1 refactoring

[luixxiul]

Though __ and _ are confusing, It looks the best option (Are there any other characters which is available for naming?). @cezaraugusto would you please add documentation on style.md and create a link on the wiki page?

However, as this will also be applied to every UI changes, it might be good not to limit the document to Aphrodite conversion.

[cezaraugusto]

ok, I'm going to add more guidelines to refactoring, being them:

1. Usage of spaces between styles objects: We should standardize it. I don't have a strong opinion.

// no space between properties:
const styles = {
  something: {
    color: 'black'
  },
  somethingElse: {
    color: 'green'
  }
}

or

// space between properties:
const styles = {
  something: {
    color: 'black'
  },

  somethingElse: {
    color: 'green'
  }
}

2. Make use of underscores (barely BEM :joy:):

browserButton: {}, // block
browserButton__icon: {}, // element
browserButton_primary: {} // modifier
browserButton_primary__icon: {} // mixing it all

3. Prefer flexbox over other block styles (self-explanatory)

4. Avoid using commonStyles Ideally, each component should have its styles together. We should use that only for a temp hack while refactoring

5. Make use of global styles only for really global matters or future-friendly theming We have a lot of things inside global.js and most shouldn't be there. We should use it for breakpoints, z-Index and colors that may change when we allow dark UI/theming. For the later, being that way we can add a flag to change styles for such cases. Like if darkMode, applydarkModeStyle`.

Once we all agree on that I'll update our styles guideline.

[luixxiul]

For 1 - I prefer the latter.

Also it would be nice to have something like csscomb for organizing styles. To enforce the guideline we need a linter anyway.

[luixxiul]

For 5 - I completely agree with that. Still I believe we should create a document of all variables, like state.md. Without such a document it would be hard to understand which variables are out there.

[cezaraugusto]

I like that idea, but I think we should make a clean-up first or at least set the foundation about "what" really should be there. ++

[cezaraugusto]

@NejcZdovc, @bsclifton what's you guys thoughts?

[cezaraugusto]

what about having variables inside the component itself? some styles are unlikely to change often (such as colors) and changing it would require a global change, for example changing Brave orange to a variant. So they should live inside global.js file.

Having things like spacing hosted there can be dangerous if, for example, two components share the same variable and at some point, we decide to change one of them. If neither the author or reviewer is aware at review time that another component depends on that variable, we have a regression and to make things worse missing it just set the style to undefined and error is silent.

thoughts?

[luixxiul]

Is it possible to create webdriver tests to avoid visual regressions?

[luixxiul]

Usage of spaces between styles objects:

I've forgot to mention a case like this:

If there is a nested object, there must be 1 blank line.

// space between properties:
const styles = {
  something: {
    color: 'black'

    ':active': {
      color: 'white'
    }
  }
}

That's the way which has been (almost) adopted on LESS files.

[luixxiul]

I started thinking that it would be worth adding something like Keep it organized- don't separate components so much that the thing would look scattered (because that is exactly what we are trying to avoid by adopting BEM). If creating components, leave notes on the BEM hierarchy.

@cezaraugusto what do you think?

[cezaraugusto]

I would go the opposite, the more components are split, the better. BEM should go when there's no easy way of doing that. It's easier to spot a bug or just read a code with that. I know it may sound confusing and open for thoughts. BEM came to rescue us from unreadable styles but IMO splitting components is the first step for a more readable codebase and we shoulnd't go other way.

[luixxiul]

I'm concerned about style inconsistency. If you separate components, the BEM naming is reset, which means you can no longer know (or imagine the possibility) that there are/would be properties which actually should be shared with other components not only in the single file but also among other files in the same folder like navigation.

Splitting the components is quite nice, still we would have to figure out how to avoid the possibility of style inconsistency. Leaving comments is my two cents.