-
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
Add a StyleProvider to support CSS-in-JS components inside iframes. #31010
Conversation
Size Change: +95 B (0%) Total Size: 1.47 MB
ℹ️ View Unchanged
|
e1263e3
to
379ce12
Compare
Reopened to continue debugging. |
f5fe1bc
to
7633796
Compare
Ok, I managed to fix the issues, it's now working properly. |
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.
This looks good to me but I worked on it so another tester would be good here :)
return createCache( { container } ); | ||
} ); | ||
|
||
export default function FrameProvider( { children, iframeDocument } ) { |
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.
Could this be renamed to emotion cache provider, perhaps?
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.
Maybe more StyleFrameProvider
or something, I don't think we should be tied to a specific library.
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.
It provides the cache right? StyleCacheProvider
? Or simply StyleProvider
.
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.
it is what makes the styles injection work, I prefer StyleProvider
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.
StyleProvider
might imply that it is necessary for basic style injection, but it's not. I think including the fact that this is specifically for iframe
s is wise, like StyleFrameProvider
or IFrameStyleProvider
if we want to be the most descriptive.
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.
What do you think @ellatrix
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.
Sure, I just wanted to point out that it's not providing an iframe but rather the style cache. Maybe StylesForIframeProvider
?
Not a blocker, I don't care that much :)
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 guess we can always rename it later since it's experimental. I don't mind "StyleProvider" personally because it probably won't break anything if used outside an iframe, it's just useless.
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 can see the style tags in the head. Nice work!
|
||
const cache = memoizedCreateCacheWithContainer( iframeDocument.head ); | ||
|
||
return <CacheProvider value={ cache }>{ children }</CacheProvider>; |
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 didn't know this existed. It's great that emotion provides this CacheProvider
and consumes it.
closes #30553
closes #28153
Testing instructions