Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Styled API #1094

Merged
merged 25 commits into from
Jun 12, 2019
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions flow-typed/mocha.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
declare function describe(string, Function): void
declare function it(string, Function): void
14 changes: 8 additions & 6 deletions packages/jss/tests/unit/createGenerateId.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

Copy link
Member Author

Choose a reason for hiding this comment

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

changes here are unrelated, but for some reason after I added tests for styled api, this tests started to fail, something weird started happening with module id, like there is a new moduleId module evaluation, I couldn't fully understand that, but I just started resetinng moduleId here.

import expect from 'expect.js'
import {createGenerateId} from '../../src'
import {resetSheets} from '../../../../tests/utils'
import * as moduleId from '../../src/utils/moduleId'

const sheetMock = {
options: {
Expand All @@ -12,20 +12,22 @@ const sheetMock = {
}

describe('Unit: jss - createGenerateId', () => {
beforeEach(resetSheets())
beforeEach(() => {
moduleId.default = 0
})

it('should return a function', () => {
expect(createGenerateId()).to.be.a(Function)
})

it('should generate a non-production class name', () => {
const generate = createGenerateId()
expect(generate({key: 'a'})).to.be('a-14-1')
expect(generate({key: 'a'})).to.be('a-0-1')
})

it('should add prefix a non-production class name', () => {
const generate = createGenerateId()
expect(generate({key: 'a'}, sheetMock)).to.be('pa-14-0-1')
expect(generate({key: 'a'}, sheetMock)).to.be('pa-0-0-1')
})

it.skip('should increment jss lib version', () => {
Expand All @@ -36,14 +38,14 @@ describe('Unit: jss - createGenerateId', () => {
it('should generate a production class name', () => {
process.env.NODE_ENV = 'production'
const generate = createGenerateId()
expect(generate()).to.be('c141')
expect(generate()).to.be('c01')
process.env.NODE_ENV = 'development'
})

it('should add prefix a production class name', () => {
process.env.NODE_ENV = 'production'
const generate = createGenerateId()
expect(generate({key: 'a'}, sheetMock)).to.be('p1401')
expect(generate({key: 'a'}, sheetMock)).to.be('p001')
process.env.NODE_ENV = 'development'
})
})
30 changes: 15 additions & 15 deletions packages/react-jss/.size-snapshot.json
Original file line number Diff line number Diff line change
@@ -1,30 +1,30 @@
{
"dist/react-jss.js": {
"bundled": 109904,
"minified": 37407,
"gzipped": 12068
"bundled": 115580,
"minified": 41857,
"gzipped": 14206
},
"dist/react-jss.min.js": {
"bundled": 85345,
"minified": 30142,
"gzipped": 9883
"bundled": 91021,
"minified": 34592,
"gzipped": 12019
},
"dist/react-jss.cjs.js": {
"bundled": 15499,
"minified": 7158,
"gzipped": 2476
"bundled": 16765,
"minified": 7614,
"gzipped": 2635
},
"dist/react-jss.esm.js": {
"bundled": 14818,
"minified": 6578,
"gzipped": 2359,
"bundled": 16038,
"minified": 6995,
"gzipped": 2513,
"treeshaked": {
"rollup": {
"code": 1946,
"import_statements": 457
"code": 1977,
"import_statements": 488
},
"webpack": {
"code": 3346
"code": 3411
}
}
}
Expand Down
4 changes: 3 additions & 1 deletion packages/react-jss/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
},
"dependencies": {
"@babel/runtime": "^7.3.1",
"@emotion/is-prop-valid": "^0.7.3",
"hoist-non-react-statics": "^3.2.0",
"jss": "10.0.0-alpha.16",
"jss-preset-default": "10.0.0-alpha.16",
Expand All @@ -50,6 +51,7 @@
"tiny-warning": "^1.0.2"
},
"devDependencies": {
"@types/react": "^16.7.20"
"@types/react": "^16.7.20",
"styled-system": "4.2.0-0"
}
}
1 change: 1 addition & 0 deletions packages/react-jss/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@ export {default as jss} from './jss'
export {SheetsRegistry, createGenerateId} from 'jss'
export {default as JssContext} from './JssContext'
export {default} from './withStyles'
export {default as styled} from './styled'
47 changes: 47 additions & 0 deletions packages/react-jss/src/styled.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// @flow
/* eslint-disable react/prop-types, react/require-default-props */

import React from 'react'
import isPropValid from '@emotion/is-prop-valid'
import type {StatelessFunctionalComponent, ComponentType} from 'react'
import withStyles from './withStyles'
import type {Options, Style, Classes} from './types'

// Props we don't want to forward.
const reservedProps = {
classes: true,
as: true,
theme: true
}

type StyledProps = {
className?: string,
as?: string,
classes?: Classes
}

export default <Props: {}>(
type: string | StatelessFunctionalComponent<Props> | ComponentType<Props>
) => {
const isTag = typeof type === 'string'

return <Theme: {}>(style: Style<Theme>, options?: Options<Theme>) => {
const Styled = (props: StyledProps) => {
const {classes, as, className} = props
const childProps: Props = ({}: any)
for (const prop in props) {
if (prop in reservedProps) continue
// We don't want to pass non-dom props to the DOM,
// but we still wat to forward them to a uses component.
if (isTag && !isPropValid(prop)) continue
childProps[prop] = props[prop]
}
// $FlowFixMe flow seems to not know that `classes` will be provided by the HOC, not by element creator.
childProps.className = className ? `${className} ${classes.css}` : classes.css
return React.createElement(as || type, childProps)
}

// $FlowFixMe flow seems to not know that `classes` will be provided by the HOC, not by element creator.
return withStyles({css: style}, {...options, injectTheme: true})(Styled)
Copy link
Member

Choose a reason for hiding this comment

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

This implementation is good for now, but I would like to optimize it, later on, to not render two extra react components for example.

I'm not sure if we should use the hooks implementation in here yet, as this would make the styled API only usable with react@16.8 currently, which in my opinion is a little bit early.

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually think performance is critical and this amount of components for each styled one is really bad, that's why I think hooks api is the only way forward with this API. For those who use older react versions they still can use the old styled implementation.

}
}
196 changes: 196 additions & 0 deletions packages/react-jss/src/styled.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,196 @@
// @flow
/* eslint-disable react/prop-types */
import expect from 'expect.js'
import React from 'react'
import TestRenderer from 'react-test-renderer'
import {stripIndent} from 'common-tags'
import {styled, SheetsRegistry, JssProvider, ThemeProvider} from '.'

const createGenerateId = () => {
let counter = 0
return rule => `${rule.key}-${counter++}`
}

describe('React-JSS: styled', () => {
it('should render static styles', () => {
const registry = new SheetsRegistry()
const Div = styled('div')({color: 'red'})
const renderer = TestRenderer.create(
<JssProvider registry={registry} generateId={createGenerateId()}>
<Div />
</JssProvider>
)
expect(registry.toString()).to.be(stripIndent`
.css-0 {
color: red;
}
`)
const {className, classes} = renderer.root.findByType('div').props
expect(className).to.be('css-0')
expect(classes).to.be(undefined)
})

it('should render dynamic styles', () => {
const registry = new SheetsRegistry()
const Div = styled('div')({
color: 'red',
width: () => 10
})
const renderer = TestRenderer.create(
<JssProvider registry={registry} generateId={createGenerateId()}>
<Div />
</JssProvider>
)
expect(registry.toString()).to.be(stripIndent`
.css-0 {
color: red;
}
.css-0-1 {
width: 10px;
}
`)
expect(renderer.root.findByType('div').props.className).to.be('css-0 css-0-1')
})

it('should merge with user class name', () => {
const registry = new SheetsRegistry()
const Div = styled('div')({color: 'red'})
const renderer = TestRenderer.create(
<JssProvider registry={registry} generateId={createGenerateId()}>
<Div className="my-class" />
</JssProvider>
)
expect(registry.toString()).to.be(stripIndent`
.css-0 {
color: red;
}
`)
expect(renderer.root.findByType('div').props.className).to.be('my-class css-0')
})

it('should use "as" prop', () => {
const registry = new SheetsRegistry()
const Div = styled('div')({color: 'red'})
const renderer = TestRenderer.create(
<JssProvider registry={registry} generateId={createGenerateId()}>
<Div as="button" />
</JssProvider>
)
expect(registry.toString()).to.be(stripIndent`
.css-0 {
color: red;
}
`)
expect(renderer.root.findByType('button').props.className).to.be('css-0')
})

it('should not leak non-dom attrs', () => {
const registry = new SheetsRegistry()
const Div = styled('div')({
color: 'red',
width: props => props.s
})
const renderer = TestRenderer.create(
<JssProvider registry={registry} generateId={createGenerateId()}>
<Div s={10} />
</JssProvider>
)
expect(registry.toString()).to.be(stripIndent`
.css-0 {
color: red;
}
.css-0-1 {
width: 10px;
}
`)
const {className, s} = renderer.root.findByType('div').props
expect(className).to.be('css-0 css-0-1')
expect(s).to.be(undefined)
})

it('should compose with styled component', () => {
const registry = new SheetsRegistry()
const BaseDiv = styled('div')({color: 'red'})
const Div = styled(BaseDiv)({width: 10})
const renderer = TestRenderer.create(
<JssProvider registry={registry} generateId={createGenerateId()}>
<Div />
</JssProvider>
)
expect(registry.toString()).to.be(stripIndent`
.css-1 {
color: red;
}
.css-0 {
width: 10px;
}
`)
const {className} = renderer.root.findByType('div').props
expect(className).to.be('css-0 css-1')
})

it('should style any component', () => {
const registry = new SheetsRegistry()
const BaseDiv = ({className}) => <div className={className} />
const Div = styled(BaseDiv)({width: 10})
const renderer = TestRenderer.create(
<JssProvider registry={registry} generateId={createGenerateId()}>
<Div />
</JssProvider>
)
expect(registry.toString()).to.be(stripIndent`
.css-0 {
width: 10px;
}
`)
const {className} = renderer.root.findByType('div').props
expect(className).to.be('css-0')
})

it.skip('should target another styled component (not sure if we really need this)', () => {
const registry = new SheetsRegistry()
const Span = styled('span')({color: 'red'})
const Div = styled('div')({
// $FlowFixMe
[Span]: {
color: 'green'
}
})

const renderer = TestRenderer.create(
<JssProvider registry={registry} generateId={createGenerateId()}>
<Div />
</JssProvider>
)
expect(registry.toString()).to.be(stripIndent`
.css-0 {
width: 10px;
}
`)
expect(renderer.root.findByType('div').props.className).to.be('XXX')
expect(renderer.root.findByType('span').props.className).to.be('XXX')
})

it('should render theme', () => {
const registry = new SheetsRegistry()
const Div = styled('div')({
color: 'red',
margin: props => props.theme.spacing
})
TestRenderer.create(
<JssProvider registry={registry} generateId={createGenerateId()}>
<ThemeProvider theme={{spacing: 10}}>
<Div />
</ThemeProvider>
</JssProvider>
)
expect(registry.toString()).to.be(stripIndent`
.css-0 {
color: red;
}
.css-0-1 {
margin: 10px;
}
`)
})
})