-
Notifications
You must be signed in to change notification settings - Fork 95
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
Replace usages of old context API #68
Conversation
Hi @xadn , thanks for this awesome PR! I'm happy to see it uses hooks, especially hooks are now in a stable react release in 16.8.0. This would have to be a breaking change though, and we need to take the cost of maintaining two versions of the lib into consideration. @ndelangen This is a breaking change. If you could chime in here it'd be great. I've also thought of introducing some new changes in a breaking release (#50), it'll be a good time to review it. |
package.json
Outdated
"react": "^16.3.1", | ||
"react-dom": "^16.3.1", | ||
"prettier": "^1.16.3", | ||
"react": "^16.8.0-alpha.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
peer dependencies need to be updated too.
src/styles/styles.js
Outdated
/** | ||
* Hook to get the component styles for the current theme. | ||
*/ | ||
export const useStyles = key => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename to baseStylesKey and add a little comment/jsdoc? Might help clarify the usage.
Probably we don't want include |
export const themeAcceptor = Component => { | ||
const ThemeAcceptor = ({ theme = DEFAULT_THEME_NAME, ...restProps }) => { | ||
const themeStyles = useMemo(() => { | ||
switch (Object.prototype.toString.call(theme)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
src/styles/styles.js
Outdated
* the current theme. This is intended to be used by the top-level inspector | ||
* components. | ||
*/ | ||
export const themeAcceptor = Component => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, maybe rename to WrappedComponent, not to confuse with React.Component
Thanks for the review, I'll fix these up today. |
…tyles.js), upgrade to stable version of react
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, will prepare for a release this weekend
🎉 |
@xadn thanks for the work, it was awesome! It's not released yet; I'll fix up some minor issues and release as 3.0.0 |
Hey guys, I really love this project, thanks for keeping it up to date! The tool I'm building sometimes does a lot of rendering which slows down the rest of the app. Unfortunately, the usage of the old context API means that the inspector component won't work with concurrent rendering and strict mode.
This PR replaces the usages of the old context API ...with hooks. Since it depends on the pre-release version of React this probably shouldn't be merged until 16.8.0.
If you're opposed to using hooks, I'd be happy to modify this to be more in-line with a different direction, but I think this ended up being pretty nice.
Changes
useStyles(key)
hook to replace thecreateStyles(key, theme)
utilthemeAcceptor
HOC for root components to take the "theme" prop and set the contextNote
To make it easier to review there are two diffs in this PR, one to run prettier on the changed files and a follow up to make the desired changes. I could redo this without prettier if needed.
Example of changes
The TH component does a good job of demonstrating what these changes look like in practice.
Old TH component (50 lines)
New TH component (34 lines)