[react-jss] Merge classes instead of overwriting #946
Conversation
HenriBeck
changed the title
packages/react-jss/src/createHoc.js
Outdated
| @@ -3,9 +3,10 @@ | |||
| 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 memoize from 'memoize-one' | |||
There was a problem hiding this comment.
do we need a lib for this? I have a feeling that function is 2-3LOC and easier to inline than to have a dependency
| * This merges to classes object. | ||
| * When a property exists in both classes, it will not overwrite but merge the two classes. | ||
| * | ||
| * @param {Object} baseClasses - The base classes. |
There was a problem hiding this comment.
we should drop jsdoc anotations since we use flow already, maintaining both is bad, because it will get out of sync without validation
|
Okay, the different scenarios would be the following:
One option would be to shallow merge the default classes with the props classes and then apply the merging of class names with the stylesheet classes. Another way would be only to use default props classes when there are no props classes. @kof what do you think? |
|
You know what? Lets do as you wanted - only shallow merge the defaultProps.classes. If component author really wants a deep merge, they can still write a utility function and do it, like we do in react-jss hoc. If we don't deeply merge it, they have at least a choice to decide how they want it. Deeply merging defaultProps.classes will have as a result those classes always added to the "class" attribute, which will increase SSR payload. Evtl. component author wants to control that behaviour. |
| @@ -0,0 +1,17 @@ | |||
| // @flow | |||
|
|
|||
| function memoize<Arg, Return>(fn: (arg: Arg) => Return) { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
* react-jss/update: (42 commits) Update jss-preset-default.md (#959) Update projects.md (#960) [core] Use tiny warning (#953) Fix peer deps warnings (#957) Docs (#952) Docs (#951) add snapshots check to pre-commit hook Docs (#950) Add migrations.md (#947) update snapshot v10.0.0-alpha.2 Update changelog.md v10.0.0-alpha.1 use jss as a name in root package.json remove abstracitons.md docs [WIP] Docs (#929) fix headings make flow happy fix tests, move regex to the top, fix global.CSS accesss ... # Conflicts: # karma.conf.js # packages/react-jss/src/compose.js
* [WIP] Update to new react context for the jss context (#924) * Update to new react context for the jss context * Fixed types * Fix some tests * Remove unnecessary cloning * Fix JssProvider * Update logic of merging context * Fix a few issues and tests * Make getTheme non public * Update warning inside JssProvider * Remove duplicate beforeEach * Update size-snapshot * Fix a few bugs * Created a Managers type * Create sheetOptions prop type definition * Removed meta property from sheet options shape * Fix creating a new generateId for every render * Move media into it's own prop * Added changelog * Rename to JssContextSubscriber * Moved context to jssContext prop instead of spreading it * [react-jss] Remove inject option (#934) * Update to new react context for the jss context * Fixed types * Fix some tests * Remove unnecessary cloning * Fix JssProvider * Update logic of merging context * Fix a few issues and tests * Make getTheme non public * Update warning inside JssProvider * Remove duplicate beforeEach * Update size-snapshot * Fix a few bugs * Remove inject option * Fix * Always inject the theme * Only inject theme when injectTheme is true * Upgrade theming package (#942) * Add forwardRef support (#943) * [react-jss] Merge classes instead of overwriting (#946) * Add forwardRef support * Add support for merging the classes * Fix path of test file * Remove jsdoc comment * Implement custom memoize-one * Convert function to arrow function * Fix * Update some of the tests * Update dynamic styles tests * Fix SSR tests * Add theme inject tests * Add merge classes tests * Remove dependency of react-dom * Fix an issue with memoizing and memoize the context * Remove createHoC file and remove injectSheet to withStyles * Update TS types * Update TS types * Fix flow types * Fix react-jss types * Rename injectSheet to withStyles in docs * Fix react-jss types * Update changelog.md * Rename class and * Fix tests * Fix a few typos in docs * Fix changelog * Use javascript for codeblocks instead of js * Upgrade theming * Fix type import * Update size-snapshot * Fix docs * Update size-snapshot * Upgrade theming to v3.0.2 * Use travis_wait for test command * Remove travis_wait command * Update docs
* [WIP] Update to new react context for the jss context (cssinjs#924) * Update to new react context for the jss context * Fixed types * Fix some tests * Remove unnecessary cloning * Fix JssProvider * Update logic of merging context * Fix a few issues and tests * Make getTheme non public * Update warning inside JssProvider * Remove duplicate beforeEach * Update size-snapshot * Fix a few bugs * Created a Managers type * Create sheetOptions prop type definition * Removed meta property from sheet options shape * Fix creating a new generateId for every render * Move media into it's own prop * Added changelog * Rename to JssContextSubscriber * Moved context to jssContext prop instead of spreading it * [react-jss] Remove inject option (cssinjs#934) * Update to new react context for the jss context * Fixed types * Fix some tests * Remove unnecessary cloning * Fix JssProvider * Update logic of merging context * Fix a few issues and tests * Make getTheme non public * Update warning inside JssProvider * Remove duplicate beforeEach * Update size-snapshot * Fix a few bugs * Remove inject option * Fix * Always inject the theme * Only inject theme when injectTheme is true * Upgrade theming package (cssinjs#942) * Add forwardRef support (cssinjs#943) * [react-jss] Merge classes instead of overwriting (cssinjs#946) * Add forwardRef support * Add support for merging the classes * Fix path of test file * Remove jsdoc comment * Implement custom memoize-one * Convert function to arrow function * Fix * Update some of the tests * Update dynamic styles tests * Fix SSR tests * Add theme inject tests * Add merge classes tests * Remove dependency of react-dom * Fix an issue with memoizing and memoize the context * Remove createHoC file and remove injectSheet to withStyles * Update TS types * Update TS types * Fix flow types * Fix react-jss types * Rename injectSheet to withStyles in docs * Fix react-jss types * Update changelog.md * Rename class and * Fix tests * Fix a few typos in docs * Fix changelog * Use javascript for codeblocks instead of js * Upgrade theming * Fix type import * Update size-snapshot * Fix docs * Update size-snapshot * Upgrade theming to v3.0.2 * Use travis_wait for test command * Remove travis_wait command * Update docs
What would you like to add/fix?
This adds support for merging classes instead of overriding them.
Currently, it is not supported to override classes by the syntax in #993.
We would need to do more work to not compute unless there is a change.
Corresponding issue (if exists): #933