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.

Sign up for GitHub

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

[react-jss] Merge classes instead of overwriting #946

Merged
merged 7 commits into from
Dec 31, 2018
Merged

[react-jss] Merge classes instead of overwriting #946

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1848151
Add forwardRef support
Dec 22, 2018
f9ec9b3
Add support for merging the classes
Dec 23, 2018
96c2687
Fix path of test file
Dec 23, 2018
9e4cde0
Remove jsdoc comment
Dec 27, 2018
85ec644
Implement custom memoize-one
Dec 27, 2018
3ada962
Convert function to arrow function
Dec 31, 2018
2763534
Merge branch 'react-jss/update' into react-jss/merge-classes
Dec 31, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion karma.conf.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ module.exports = config => {
'./packages/react-jss/tests/theming.js',
'./packages/react-jss/tests/dynamic-styles.js',
'./packages/react-jss/src/index.test.js',
'./packages/react-jss/src/compose.test.js',
'./packages/react-jss/src/merge-classes.test.js',
'./packages/react-jss/src/JssProvider.test.js',
'./packages/react-jss/src/injectSheet.test.js'
],
Expand Down
30 changes: 15 additions & 15 deletions packages/react-jss/.size-snapshot.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,30 +1,30 @@
{
"dist/react-jss.js": {
"bundled": 146196,
"minified": 48865,
"gzipped": 14339
"bundled": 146062,
"minified": 49087,
"gzipped": 14328
},
"dist/react-jss.min.js": {
"bundled": 111372,
"minified": 38169,
"gzipped": 11622
"bundled": 111238,
"minified": 38389,
"gzipped": 11598
},
"dist/react-jss.cjs.js": {
"bundled": 15227,
"minified": 6630,
"gzipped": 2259
"bundled": 14321,
"minified": 6509,
"gzipped": 2150
},
"dist/react-jss.esm.js": {
"bundled": 14671,
"minified": 6180,
"gzipped": 2158,
"bundled": 13670,
"minified": 5957,
"gzipped": 2046,
"treeshaked": {
"rollup": {
"code": 1712,
"import_statements": 492
"code": 1647,
"import_statements": 472
},
"webpack": {
"code": 3130
"code": 3031
}
}
}
Expand Down
32 changes: 0 additions & 32 deletions packages/react-jss/src/compose.js
View file
Open in desktop

This file was deleted.

76 changes: 35 additions & 41 deletions packages/react-jss/src/createHoc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,13 @@
import React, {Component, type ComponentType} from 'react'
import PropTypes from 'prop-types'
import {ThemeContext} from 'theming'
import {getDynamicStyles, SheetsManager, type StyleSheet, type Classes} from 'jss'
import {getDynamicStyles, SheetsManager, type StyleSheet} from 'jss'
import defaultJss from './jss'
import compose from './compose'
import mergeClasses from './merge-classes'
import getDisplayName from './getDisplayName'
import JssContext from './JssContext'
import type {Options, Theme, StylesOrCreator, InnerProps, OuterProps} from './types'
import memoize from './memoize-one'

const env = process.env.NODE_ENV

Expand Down Expand Up @@ -68,9 +69,6 @@ export default function createHOC<
const manager = new SheetsManager()
const ThemeConsumer = (theming && theming.context.Consumer) || ThemeContext.Consumer

// $FlowFixMe: DefaultProps is missing in type definitions
const {classes: defaultClasses = {}, ...defaultProps} = {...InnerComponent.defaultProps}

const getTheme = props => (isThemingEnabled && props.theme ? props.theme : noTheme)

class Jss extends Component<OuterPropsType, State> {
Expand All @@ -80,7 +78,14 @@ export default function createHOC<
innerRef: PropTypes.func
}

static defaultProps = defaultProps
// $FlowFixMe
static defaultProps = {...InnerComponent.defaultProps}

mergeClassesProp = memoize(classesProp => {
const {classes} = this.state

return classesProp ? mergeClasses(classes, classesProp) : classes
})

constructor(props: OuterPropsType) {
super(props)
Expand Down Expand Up @@ -198,20 +203,6 @@ export default function createHOC<
}
}

computeClasses(staticSheet: StyleSheet, dynamicSheet?: StyleSheet): Classes {
const jssClasses = dynamicSheet
? compose(
staticSheet.classes,
dynamicSheet.classes
)
: staticSheet.classes
return {
...defaultClasses,
...jssClasses,
...this.props.classes
}
}

createState(): State {
if (this.props.jssContext.disableStylesGeneration) {
return {classes: {}}
Expand All @@ -223,22 +214,22 @@ export default function createHOC<
return {
staticSheet,
dynamicSheet,
classes: this.computeClasses(staticSheet, dynamicSheet)
classes: mergeClasses(staticSheet.classes, dynamicSheet ? dynamicSheet.classes : {})
kof marked this conversation as resolved.
Show resolved Hide resolved
}
}

render() {
const {classes} = this.state
const {
innerRef,
jssContext,
theme,
classes,
// $FlowFixMe: Flow complains for no reason...
...props
} = this.props

// We have merged classes already.
props.classes = classes
// Merge the class names for the user into the sheet classes
props.classes = this.mergeClassesProp(classes)

if (innerRef) props.ref = innerRef
if (injectTheme) props.theme = theme
Expand All @@ -247,21 +238,24 @@ export default function createHOC<
}
}

return function JssContextSubscriber(props) {
return (
<JssContext.Consumer>
{context => {
if (isThemingEnabled || injectTheme) {
return (
<ThemeConsumer>
{theme => <Jss theme={theme} {...props} jssContext={context} />}
</ThemeConsumer>
)
}

return <Jss {...props} jssContext={context} />
}}
</JssContext.Consumer>
)
}
// $FlowFixMe: Sadly there is no support for forwardRef yet
const JssContextSubscriber = React.forwardRef((props, ref) => (
<JssContext.Consumer>
{context => {
if (isThemingEnabled || injectTheme) {
return (
<ThemeConsumer>
{theme => <Jss innerRef={ref} theme={theme} {...props} jssContext={context} />}
</ThemeConsumer>
)
}

return <Jss innerRef={ref} {...props} jssContext={context} />
}}
</JssContext.Consumer>
))

JssContextSubscriber.displayName = 'JssContextSubscriber'

return JssContextSubscriber
}
17 changes: 17 additions & 0 deletions packages/react-jss/src/memoize-one.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// @flow

function memoize<Arg, Return>(fn: (arg: Arg) => Return) {
Copy link
Member

Choose a reason for hiding this comment

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

btw why do you like named functions so much? by the latest spec a function expression assigned to a variable has a function name as well, derived from the variable name. So there is no problem with debugging.

I don't like named functions because of the implicit hoisting, its a generally leaky concept.

Copy link
Member

Choose a reason for hiding this comment

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

its like var and even worse, no one is using var any more and named functions are even worse, because they can be called before they are defined and once people start relying on this, code gets messy.

Copy link
Member Author

Choose a reason for hiding this comment

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

Personally, it makes it easier to differentiate between variables and I'm always using eslint or tslint where I disallow to not use something before it's not defined.

let lastArg
let lastResult

return (arg: Arg): Return => {
if (typeof lastArg === 'undefined' || lastArg !== arg) {
lastArg = arg
lastResult = fn(arg)
}

return lastResult
}
}

export default memoize
17 changes: 17 additions & 0 deletions packages/react-jss/src/merge-classes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// @flow
import type {Classes} from 'jss'

function mergeClasses(baseClasses: Classes, additionalClasses: Classes) {
const combinedClasses = {...baseClasses}

for (const name in additionalClasses) {
combinedClasses[name] =
name in combinedClasses
? `${combinedClasses[name]} ${additionalClasses[name]}`
: additionalClasses[name]
}

return combinedClasses
}

export default mergeClasses
6 changes: 3 additions & 3 deletions packages/react-jss/src/compose.test.js → packages/react-jss/src/merge-classes.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import expect from 'expect.js'
import compose from './compose'
import compose from './merge-classes'

describe('compose', () => {
it('should compose two class objects', () => {
describe('react-jss: merge-classes', () => {
it('should merge two class objects', () => {
const staticClasses = {
a: 'a',
b: 'b'
Expand Down