[react-jss] Remove inject option #934
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 17 commits
November 26, 2018 21:55
…t-jss/jss-context * origin/react-jss/jss-context: Fix hoisting of new react features (#925)
kof
reviewed
Dec 13, 2018
packages/react-jss/src/createHoc.js
Outdated
|
|
||
| if (injectMap.sheet && !props.sheet && sheet) props.sheet = sheet | ||
| if (injectMap.theme) props.theme = theme | ||
| if (injectTheme && props.theme) props.theme = theme |
There was a problem hiding this comment.
I am not sure we even need injectTheme prop
There was a problem hiding this comment.
Actually always injecting theme shouldn't be a big problem, the theme shouldn't change often that it would cause a lot of rerenders
There was a problem hiding this comment.
I would say yes, btw that line doesn't make sense, bc you have no theme in props, props is the rest ... so we seem to lack a test for this
There was a problem hiding this comment.
We have a test for that, the tests are failing anyway right now
kof
reviewed
Dec 13, 2018
| @@ -25,7 +25,7 @@ export const JssProvider: React.ComponentType<{ | |||
| type Omit<T, K> = Pick<T, Exclude<keyof T, K>> | |||
| type Options = { | |||
| index?: number | |||
| inject?: Array<'classes' | 'theme' | 'sheet'> | |||
| injectTheme?: boolean | |||
There was a problem hiding this comment.
so we can remove injectTheme completely?
There was a problem hiding this comment.
No, someone might still want to subscribe to the theme even though he has a styles object
kof
reviewed
Dec 13, 2018
| @@ -76,8 +60,7 @@ export default function createHOC< | |||
| options: Options | |||
| ): ComponentType<OuterPropsType> { | |||
| const isThemingEnabled = typeof stylesOrCreator === 'function' | |||
There was a problem hiding this comment.
I think we can also get rid of isThemingEnabled, because if a theme is on the context, its weird it would be implicitely depending on styles being a function
There was a problem hiding this comment.
No, we still need to do some checks based on isThemingEnabled
There was a problem hiding this comment.
Lets revisit them, I think we can get rid of it.
There was a problem hiding this comment.
We still need this to decide which theme we are using.
When someone passed a function as the styles, we use the theme prop.
Because the user still might pass injectTheme: true which would subscribe to the theme, even though the styles are not theme dependent, we would create additional stylesheets.
There was a problem hiding this comment.
I think isThemingEnabled logic needs to be different, e.g. shouldSubscribeTheme = options.injectTheme || stylesIsfunction and rendering of styles just based on stylesIsFunction
kof
added a commit
that referenced
this pull request
Jan 16, 2019
* [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
This was referenced Jan 20, 2019
bhupinderbola
pushed a commit
to bhupinderbola/jss
that referenced
this pull request
Sep 17, 2019
* [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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What would you like to add/fix?
This removes the
injectoption for the HOC and replaces it with only one option for injecting the theme.Injecting the
sheetshouldn't be necessary and unstable because it would be sometimes the dynamic sheet if it exists or the static sheet.And there is no real benefit to not injecting the
classesprop.Injecting the theme is now handled by the option
injectThemewhich is a boolean.