Add JSS example #23
Add JSS example #23
Conversation
|
Thanks, will do. |
| fontSize: '1.25rem', | ||
| fontWeight: '300', | ||
| lineHeight: '1.5em', | ||
| margin: '0', |
| const styles = { | ||
| text: { | ||
| fontSize: '1.25rem', | ||
| fontWeight: '300', |
| }, | ||
| }; | ||
|
|
||
| const App = (props) => ( |
There was a problem hiding this comment.
a bit nicer notation (but really stylistic preference)
const App = ({sheet: {classes}}) => (
...
<div className={classes.container}>
| }, | ||
| }; | ||
|
|
||
| const Content = ({ text, media, sheet }) => ( |
There was a problem hiding this comment.
same here, looks like much less repition to me if you spread the sheet and just pick classes ... you rarely need the sheet ref:
+const Content = ({ text, media, sheet: {classes} }) => (
|
|
||
| const Content = ({ text, media, sheet }) => ( | ||
| <div> | ||
| <p className={sheet.classes.text} dangerouslySetInnerHTML={{ __html: text }} /> |
There was a problem hiding this comment.
not sure if I need to comment on this, because not related to jss, but looks like something can be done differently ...
| textTransform: 'uppercase', | ||
| }, | ||
| value: { | ||
| fontWeight: '700', |
| }, | ||
| }; | ||
|
|
||
| class Footer extends Component { |
There was a problem hiding this comment.
why not to use decorator
@useSheet(styles)
There was a problem hiding this comment.
Not a fan of decorators since they aren't standardized yet and rely on a legacy babel transform. 😁 See this dicussion: facebook/create-react-app#107
Specifically this part:
Decorators are currently at "stage 1" in the ecmascript-standardization process, and it isn't until "stage 3" that all the syntax and semantics are complete, so I think it makes sense to wait.
| this.handleClick = this.handleClick.bind(this); | ||
| } | ||
|
|
||
| handleClick() { |
There was a problem hiding this comment.
why not to use this syntax so you don't need to bind:
handleClick = () => {}
There was a problem hiding this comment.
Good idea, but need to update all examples. Will add to #24!
| }, | ||
| name: { | ||
| color: '#292f33', | ||
| fontWeight: '700', |
|
|
||
| jss.setup(preset()); | ||
|
|
||
| const globalStyles = { |
There was a problem hiding this comment.
would probably put this global styles and its rendering into a separate module.
There was a problem hiding this comment.
so that this file has a focus on app setup/initialization.
| // Attach global styles | ||
| jss.createStyleSheet(globalStyles, { named: false }).attach(); | ||
|
|
||
| import App from './components/app'; |
There was a problem hiding this comment.
imports should be on top, because they are hoisted anyways ... this may lead to a wrong understanding of execution order.
| }, | ||
| '@media screen and (min-width: 600px)': { | ||
| html: { | ||
| fontSize: '16px', |
There was a problem hiding this comment.
jss-default-unit allows us to have numeric values here as well, px is added automatically.
There was a problem hiding this comment.
there are 2 more cases of this in this sheet.
|
What I would love to see as well is using a separate module with constants for colors, because it shows the possibilities. |
There was a problem hiding this comment.
Also I would love to see the use of color package https://www.npmjs.com/package/color
Because it is not obvious to every one that things like darken() etc are already possible.
| fill: 'currentColor', | ||
| height: '1.25em', | ||
| }, | ||
| '@media screen and (min-width: 360px)': { |
There was a problem hiding this comment.
to show the beauty of js, you could put all media queries into a separate file and then use them like this:
...
[screenSizes.xs]: {
}
...
There was a problem hiding this comment.
Again, would need to update for all CSS-in-JS examples – will add to #24!
| }, | ||
| button: { | ||
| display: 'flex', | ||
| flexGrow: '1', |
| }, | ||
| icon: { | ||
| display: 'flex', | ||
| flexGrow: '1', |
|
Awesome, thanks for going through this! Will update soon |
/cc @kof who might want to review this
Closes #1