Skip to content
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(example): add an example of using emotion css prop #1607

Closed
wants to merge 8 commits into from
Closed

docs(example): add an example of using emotion css prop #1607

wants to merge 8 commits into from

Conversation

himorishige
Copy link
Contributor

This is an example of using Emotion css prop.

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Jan 22, 2022

Hi @himorishige,

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 CLA Signed label will be added to the pull request.

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

@remix-cla-bot
Copy link
Contributor

remix-cla-bot bot commented Jan 22, 2022

Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳

@@ -0,0 +1,25 @@
/** @jsx jsx */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does esbuild respect this pragma?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

esbuild supports jsx annotations.
However, it seems that jsxImportSource support is not yet implemented without babel support.

ref:
Support JSX annotation
evanw/esbuild#138

Support /** @jsx pragma
evanw/esbuild#389

Add support for jsxImportSource
evanw/esbuild#1172

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although it is limited, it seems to be possible to remove the pragma by adopting the patch in this issue.

<meta name="viewport" content="width=device-width,initial-scale=1" />
<Meta />
<Links />
{typeof document === "undefined" ? "__STYLES__" : null}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you confident this doesn't result in hydration mismatches? What about client side navigations, does emotion replace the contents here? Is it stable on rerenders in App?

Copy link
Contributor Author

@himorishige himorishige Jan 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review.
There were some implementation mistakes...
I've committed the adjusted version again.

@himorishige
Copy link
Contributor Author

@ryanflorence
Copy link
Member

Awesome, can you add another route and link to it in so we can navigate back and forth to make sure it works on transitions?

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this!

In addition to Ryan's comments, could you also check that a root ErrorBoundary and CatchBoundary can be used without completely blowing away the styles?

Thank you!

@himorishige
Copy link
Contributor Author

Thanks for the confirmation and suggestions.
I created the page links, CacheBoundary and ErrorBaundary.
The style didn't seem to go wrong when going forward or backward in the page.

https://codesandbox.io/s/zealous-danny-3gmpp?file=/app/routes/index.tsx

If we open a page that doesn't exist directly also for 404 pages, the styles are reflected, but jumping from a link seemed to blow the styles...

Maybe the reason is that in entry.client.ts, the target to hydrate includes the root component but not styles-context.tsx.

I'll think about it some more.
Do you know of any good suggestions for this part?

@kentcdodds
Copy link
Member

Thank you for investigating that!

Do you know of any good suggestions for this part?

I'm afraid I don't. I haven't really thought much about it. It seems like we may need a way to store the virtual style sheet outside the React tree and reuse it when rendering the document for the app and the root-level boundaries. Not sure whether the css-in-js libraries have this ability though.

@himorishige
Copy link
Contributor Author

Thank you so much! I understand.
We may need to consider how to use it on the css-in-js side as well. I'll try to think of a better way.

@kentcdodds
Copy link
Member

One other thing, there is already a PR for emotion support and I'd prefer to avoid having two examples of basically the same thing. Maybe we can try and get the css prop working in that example (if it does?). Would you mind collaborating over there please? #1485

@himorishige
Copy link
Contributor Author

Thank you. Sorry, I hadn't noticed the other example...
I'll close this one and support the other PR.

@osdiab
Copy link

osdiab commented Sep 6, 2022

Hello,

Sorry to bring this back to life but the emotion example doesn't actually use the CSS prop, it just uses the @emotion/styled API (which I do not prefer). I'm still trying to figure out how to get the CSS prop to work without typescript freakouts (right now it gets angry when i use the /* @jsx jsx */ pragma, saying that the jsx import is never used despite it being used in the pragma), would there be interest in augmenting that example to include CSS prop usage? Thanks!

EDIT: i just got around it by using ESLint to warn on unused variables instead of TypeScript itself and that makes it happy, as evidently typescript-eslint handles the pragma better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants