Skip to content
This repository has been archived by the owner. It is now read-only.

Add style standard to style doc #9177

Merged
merged 2 commits into from Jun 3, 2017
Merged

Add style standard to style doc #9177

merged 2 commits into from Jun 3, 2017

Conversation

@cezaraugusto
Copy link
Contributor

cezaraugusto commented May 31, 2017

Auditors: @luixxiul
Close #8340

We have standardjs to take care of coding practices but we're still loose about how we deal with styles. As codebase grows it gets harder to follow a convention. This update adds new coding practices while writing styles and enforce using our adapted BEM pattern.

@NejcZdovc
Copy link
Member

NejcZdovc commented May 31, 2017

@cezaraugusto can we put this into linter? eslint custom rule maybe?

@cezaraugusto
Copy link
Contributor Author

cezaraugusto commented May 31, 2017

I was thinking about that yesterday, a custom eslint rule would be ideal. We can take StyleSheet.create as the identifier and work from there looking for spaces/quotes and other rules we want to check.

Didn't have time to look closer but worth considering, so ++. I created an issue to track this #9181

@luixxiul
Copy link
Contributor

luixxiul commented Jun 2, 2017

I would add the reason why we are going to choose BEM over other methodologies (1. following BEM should avoid visual regressions nicely, 2. BEM makes it possible for us to make preparations for style nesting in advance, though it is not sure if it could be implemented to Aphrodite).

Also I found a typo on the file. I'm going to fix it and push the fix.

@luixxiul
Copy link
Contributor

luixxiul commented Jun 2, 2017

Copy link
Contributor

luixxiul left a comment

Feedback left.

Auditors: @luixxiul
Close #8340
auditors: @luixxiul
@cezaraugusto
Copy link
Contributor Author

cezaraugusto commented Jun 2, 2017

thanks, added some reasons on why we choose BEM, feel free to edit if you have more/better ones

Copy link
Contributor

luixxiul left a comment

the instruction looks nice!

@cezaraugusto cezaraugusto merged commit d4f169a into brave:master Jun 3, 2017
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@cezaraugusto cezaraugusto deleted the cezaraugusto:docs/style/8340 branch Jun 3, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.