This repository has been archived by the owner on Dec 11, 2019. It is now read-only.
[Proposal] CSS in JS documentation #5205
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,141 @@ | ||
| # Overview | ||
|
|
||
| All applicable styles should be colocated with their corresponding JavaScript component using [Aphrodite](https://github.com/Khan/aphrodite). | ||
|
|
||
| ## Legacy | ||
|
|
||
| All legacy styles are processed with [Less](http://lesscss.org/) and can be found in the /less directory. These should still be maintained but all future CSS should be written in JavaScript and kept inside the appropriate component file. | ||
|
|
||
| ## Example | ||
|
|
||
| Here is an example of a React component styled with Aphrodite. | ||
|
|
||
| Aphrodite will automatically create a single `<style>` tag in the document `<head>` to put its generated styles in. | ||
|
|
||
| ```jsx | ||
| const React = require('react') | ||
| const ImmutableComponent = require('./immutableComponent') | ||
| const {StyleSheet, css} = require('aphrodite') | ||
|
|
||
| class Button extends ImmutableComponent { | ||
| render () { | ||
| return <span className={css(styles.browserButton)} | ||
| disabled={this.props.disabled} | ||
| data-l10n-id={this.props.l10nId} | ||
| data-button-value={this.props.dataButtonValue} | ||
| onClick={this.props.onClick} /> | ||
| } | ||
| } | ||
|
|
||
| const styles = StyleSheet.create({ | ||
|
There was a problem hiding this comment. Side note is we could probably hook this easily in the future if KA didn't already do this and inspect the style properties and strings and do certain checks and console.warn for problems. There was a problem hiding this comment. @bbondy that is a great point- we should be able to do testing for important styles 😄 There was a problem hiding this comment. Yeah totally. My only point with "linting" was that Aphrodite doesn't have a robust ecosystem of tools available like PostCSS does. |
||
| browserButton: { | ||
| cursor: 'default', | ||
| display: 'inline-block', | ||
| lineHeight: '25px', | ||
| width: '25px', | ||
| height: '25px', | ||
|
There was a problem hiding this comment. Can you make a recommendation on units as well as part of this documentation? Should we avoid px for example on new stuff or is it ok? We seem to have some support issues along the lines of things look tiny, maybe due to dpi? Not sure but just wondering if this type of thing could be handled at the same time as we do a new style system. Now would be a great time to figure that type o thing out and make sure new things use the new system. |
||
|
|
||
| ':hover': { | ||
| color: 'black' | ||
| }, | ||
|
|
||
| '@media (min-width: 500px)': { | ||
| width: '100px' | ||
| } | ||
| } | ||
| }) | ||
|
|
||
| module.exports = Button | ||
| ``` | ||
|
|
||
| A few things to note: | ||
|
|
||
| 1. All multi-word properties become camel cased (lineHeight instead of line-height). | ||
| 2. Pseudo selectors and media queries can be given their own class or be nested within a selector. | ||
|
|
||
| ## Sharing Styles | ||
|
|
||
| Shared styles go in the `app/styles` directory. For example, we could create a Buttons.js file in `app/styles` with something like: | ||
|
|
||
| ```jsx | ||
| import { StyleSheet } from 'aphrodite' | ||
|
|
||
| export default StyleSheet.create({ | ||
| Button: { | ||
| background: 'red' | ||
| } | ||
| }) | ||
| ``` | ||
|
|
||
| And then in a component file we could: | ||
|
|
||
| ```jsx | ||
| import ButtonStyles from './styles/Buttons' | ||
|
|
||
| ... | ||
|
|
||
| <button className={css(ButtonStyles.Button)}>Click Me</button> | ||
| ``` | ||
|
|
||
|
|
||
| ### Passing Styles to Child Components | ||
|
|
||
| Alternatively, styles can be treated as props and passed down from parent to child. Here is an example: | ||
|
|
||
| ```jsx | ||
| // addEditBookmarkHanger.js | ||
|
|
||
| const React = require('react') | ||
| const ImmutableComponent = require('../../../js/components/immutableComponent') | ||
| const Button = require('../../../js/components/button') | ||
| const {StyleSheet, css} = require('aphrodite') | ||
|
|
||
| class AddEditBookmarkHanger extends ImmutableComponent { | ||
| render () { | ||
| <Button styles={[styles.hangerButton, styles.hangerText]} /> | ||
| } | ||
| } | ||
|
|
||
| const styles = StyleSheet.create({ | ||
| hangerButton: { | ||
| width: '50px', | ||
| height: '25px', | ||
|
|
||
| ':hover': { | ||
| color: orange | ||
| } | ||
| }, | ||
|
|
||
| hangerText: { | ||
| fontSize: '20px' | ||
| } | ||
| }) | ||
|
|
||
| // button.js | ||
|
|
||
| const React = require('react') | ||
| const ImmutableComponent = require('./immutableComponent') | ||
| const {StyleSheet, css} = require('aphrodite') | ||
|
|
||
| class Button extends ImmutableComponent { | ||
| render () { | ||
| return <span className={css(this.props.styles)} /> | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ## Testing | ||
|
|
||
| As styles are converted from LESS to Aphrodite, we will lose the ability to control the generated class names. This means for our webdriver tests will no longer be able to use class names as selectors. Instead we will be adding the attribute `data-test-id` to elements. This approach gives us the added benefit of differentiating between code meant for presentation and code meant for testing. | ||
|
|
||
| An example would be moving: | ||
|
|
||
| ```jsx | ||
| <Button className='paymentHistoryButton' l10nId={buttonText} onClick={onButtonClick.bind(this)} /> | ||
| ``` | ||
|
|
||
| to: | ||
|
|
||
| ```jsx | ||
| <Button className={css(paymentButton)} data-test-id='paymentHistoryButton' l10nId={buttonText} onClick={onButtonClick.bind(this)} /> | ||
| ``` | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about for example our Dialogs.
https://github.com/brave/browser-laptop/blob/master/less/forms.less#L100
https://github.com/brave/browser-laptop/blob/master/less/forms.less#L144
https://github.com/brave/browser-laptop/blob/master/less/forms.less#L194
...
It's a lot of repeated styles and could benefit from shared re-use. I'm thinking in those cases we jus require in a shared common style from app/renderer/styles/*.js?