-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
docs(examples): Add Emotion example #1485
Conversation
Hi @helderburato, Welcome, and thank you for contributing to Remix! Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once. You may review the CLA and sign it by adding your name to contributors.yml. Once the CLA is signed, the If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at [email protected]. Thanks! - The Remix team |
Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳 |
Co-authored-by: Michaël De Boey <[email protected]>
Hi, can you test if this implementation works with Cloudflare Worker? I try it, but it's appearing that emotion is calling some packages that don't work in a worker |
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.
Thanks for taking the time here!
Could you add one or two routes to make sure that it works across those route boundaries? And double-check that CatchBoundary
and ErrorBoundary
on the root route will work because I'm not certain it will.
Hey @kentcdodds! If I catch this well, I added two new routes The styles still working through the routes. Ps.: I tested it in an incognito tab and working well, could you check it again? @kentcdodds, If it's ok, I'll update example #1495 to the same approach. |
91fd98f
to
66bc529
Compare
My big concern is that if there's a CatchBoundary or an ErrorBoundary and those are rendered due to an error, all the styles will be removed because the |
If I catch it well, I triggered a catch/error boundary into the And the styles persist into the nested routes. Is that what you expect @kentcdodds ? If so, I can apply the same to the #1495 |
What if you have a |
Here are more details on the issue with CSS-in-JS: #1512 (comment) |
Hi @kentcdodds, I updated the project at the commit 7894b95. The behavior looks like this: Root ErrorCSS RenderedIs that the expected? |
If it's server rendered then it works fine. But it breaks for client transitions. Can you try doing the same thing I'm doing in that video? |
I was able to reproduce the problem and it actually breaks the client, like with other CSS-in-JS libraries. @helderburato, let me know if you want me to make a PR to your branch. |
What do you think about @kentcdodds ? |
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 is awesome! Could you have a section in the README that describes how the example works and why you need to interact with the cache? Also make reference to the relevant files in the example. Then this will be ready to merge. Thanks!
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.
Super 👍
This is very helpful. Thank you 👏👏
No description provided.