Skip to content
This repository has been archived by the owner on Jan 27, 2019. It is now read-only.

Add JSS example #23

Merged
merged 3 commits into from
Sep 21, 2016
Merged

Add JSS example #23

merged 3 commits into from
Sep 21, 2016

Conversation

mxstbr
Copy link
Member

@mxstbr mxstbr commented Sep 21, 2016

/cc @kof who might want to review this

Closes #1

@kof
Copy link
Contributor

kof commented Sep 21, 2016

Thanks, will do.

fontSize: '1.25rem',
fontWeight: '300',
lineHeight: '1.5em',
margin: '0',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can use a number here

const styles = {
text: {
fontSize: '1.25rem',
fontWeight: '300',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can use a number here

},
};

const App = (props) => (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a bit nicer notation (but really stylistic preference)

const App = ({sheet: {classes}}) => (
...

 <div className={classes.container}>

},
};

const Content = ({ text, media, sheet }) => (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 }} />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be a number

},
};

class Footer extends Component {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not to use decorator

@useSheet(styles)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not to use this syntax so you don't need to bind:

handleClick = () => {}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, but need to update all examples. Will add to #24!

},
name: {
color: '#292f33',
fontWeight: '700',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be a number


jss.setup(preset());

const globalStyles = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would probably put this global styles and its rendering into a separate module.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jss-default-unit allows us to have numeric values here as well, px is added automatically.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are 2 more cases of this in this sheet.

@kof
Copy link
Contributor

kof commented Sep 21, 2016

What I would love to see as well is using a separate module with constants for colors, because it shows the possibilities.

Copy link
Contributor

@kof kof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)': {
Copy link
Contributor

@kof kof Sep 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to show the beauty of js, you could put all media queries into a separate file and then use them like this:

...

[screenSizes.xs]: {
}
...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, would need to update for all CSS-in-JS examples – will add to #24!

},
button: {
display: 'flex',
flexGrow: '1',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be a number

},
icon: {
display: 'flex',
flexGrow: '1',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be a number

@mxstbr
Copy link
Member Author

mxstbr commented Sep 21, 2016

Awesome, thanks for going through this! Will update soon

@mxstbr
Copy link
Member Author

mxstbr commented Sep 21, 2016

Pushed commits with fixes for the suggestions, will merge for now! Let's do #24 and #25 and iterate on all of the examples with more general fixes.

Thanks for reviewing!

@mxstbr mxstbr merged commit 892871e into master Sep 21, 2016
@mxstbr mxstbr deleted the jss branch September 21, 2016 18:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants