marigold-ui / marigold

Design System based on react-aria and Tailwind CSS
https://marigold-ui.io
MIT License
101 stars 7 forks source link

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

Closed ti10le closed 2 years ago

ti10le commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

'@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 commented 2 years ago

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 commented 2 years ago

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

viktoria-schwarz commented 2 years ago

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 commented 2 years ago

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

sebald commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

moved everthing here: https://github.com/marigold-ui/marigold/discussions/1372