-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
components: Add useCx #33172
components: Add useCx #33172
Conversation
Size Change: +12.4 kB (+1%) Total Size: 1.06 MB
ℹ️ View Unchanged
|
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.
Very neat!
I wonder if we should at least add some JSDocs to show a usage example and explain why this hook exists in the first place.
Also, out of curiosity, would mixing different Emotion's "techniques" (css
, styles
, useCx
) work and access the same cache?
import { useContext, useCallback } from '@wordpress/element'; | ||
|
||
// @ts-ignore Private property | ||
const EmotionCacheContext: Context< EmotionCache > = CacheProvider._context; |
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.
We may not need to access a private property if emotion-js/emotion#2418 gets merged 🤞
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.
Yeah. Unfortunately it seems that my PR isn't gaining any traction.
Looking at the React internals, they seem to use this property a lot, so I would be surprised if it went away completely in the future: https://github.com/facebook/react/search?q=._context
...classNames.map( ( arg ) => { | ||
if ( isSerializedStyles( arg ) ) { | ||
insertStyles( cache, arg, false ); | ||
return `${ cache.key }-${ arg.name }`; |
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.
Is there any chance that this interpolation technique may change in emotion and potentially break this hook?
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.
I doubt it, but we'd notice it quickly I would think. Testing that iframes are working with Emotion should be something we do every Emotion upgrade.
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.
Testing that iframes are working with Emotion
This is actually a good point. Should we also add a unit test and/or storybook story to showcase iframe compatibility?
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.
Oh yeah, that'd be good to do, I'll add it.
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.
I'm not sure if we should wait for some feedback from the Emotion's team before merging, but otherwise these changes LGTM
* components: Add useCx * Add tests * Add JSDoc * Add story for useCx
Description
Adds a
useCx
hook which creates acx
function that is able to take inSerializedStyles
returned by thecss
function from@emotion/react
. It also attaches these to the current EmotionCache meaning that it will work with iframes.How has this been tested?
Unit tests and storybook.
Types of changes
New feature.
Checklist:
*.native.js
files for terms that need renaming or removal).