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

bug: css class order changes at using useTheme in MarigoldProvider #1317

Closed
ti10le opened this issue Sep 15, 2021 · 9 comments
Closed

bug: css class order changes at using useTheme in MarigoldProvider #1317

ti10le opened this issue Sep 15, 2021 · 9 comments
Assignees
Labels
status:ready Ready for development type:bug Something isn't working

Comments

@ti10le
Copy link
Contributor

ti10le commented Sep 15, 2021

Description

The css class order changes if you are using useTheme() hook in MarigoldProvider. (this was recognized at #1197)

How to reproduce

Steps to reproduce the behavior:

  1. use useTheme in MarigoldProvider (like with the GlobalStyles in the linked issue)
  2. make gatsby build
  3. gatsby serve
  4. See that the style order is not the same as local devmode or in current live state.

Expected behavior

Css classes should be the same order in local dev mode, local production (gatsby serve) and live.

Screenshots

image
Bildschirmfoto 2021-09-15 um 09 54 05

@ti10le ti10le added the type:bug Something isn't working label Sep 15, 2021
@ti10le
Copy link
Contributor Author

ti10le commented Sep 15, 2021

The variant and custom styles are overridden by reset.base styles (css class css-ase7ea). The order is changed (current guess) by using the useTheme hook in the MarigoldProvider.

@ti10le
Copy link
Contributor Author

ti10le commented Sep 16, 2021

'@emotion/react' and '@emotion/css' are not good at working together. We found out that this combination changes the order of the css styles. One solution is to remove the css() function from emotion/css which is used in reset.ts
After we removed that and changed the useStyles hook a little bit we got a production build which are the same as in dev mode.

@ti10le
Copy link
Contributor Author

ti10le commented Sep 22, 2021

Still trying to find a solution for this. The current problem is that the GlobalStyles cannot be replaced by changing the theme. You should set GlobalStyles which are replaced by another theme globals (not added)

@ti10le ti10le self-assigned this Sep 22, 2021
@ti10le
Copy link
Contributor Author

ti10le commented Sep 28, 2021

We found out that the best way is to deepmerge alle styles inside useStyles or inside useClassname.

@viktoria-schwarz
Copy link
Contributor

I would definitely go for option 3, a.k.a. putting everything in the correct order with a deepmerge in useStyles.
In my understanding we need to merge the reset.base (base styles) with the reset styles of the given element and this outputs the resetClassName. Same with base and customClassName that outputs the new customClassName.

@ti10le
Copy link
Contributor Author

ti10le commented Oct 6, 2021

I would also go for this option.
What are your thoughts @sebald ?

@sebald
Copy link
Member

sebald commented Oct 6, 2021

@viktoria-schwarz @ti10le that option doesn't use a deep merge, if you referring to this doc: https://github.com/marigold-ui/marigold/blob/global-css-with-separate-caches/packages/system/src/issues.md

The deepmerge is causing the problem. It solves the ordering issue and creates an issue with overriding props. See example.


Option: is basically the current implementation, but making sure the styles are inverted in the correct order. And the question remains: how!

@ti10le
Copy link
Contributor Author

ti10le commented Oct 6, 2021

Ok as I see it we have one option where we don't have string classnames anymore and use css prop and two options where we have to clarify an important open question - how do we implement this. 🤔

@sebald
Copy link
Member

sebald commented Oct 6, 2021

@ti10le ti10le added the status:ready Ready for development label Oct 8, 2021
@ti10le ti10le closed this as completed Dec 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:ready Ready for development type:bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants