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

CSS order issue. #913

Open
hrasoa opened this issue Jan 2, 2022 · 14 comments
Open

CSS order issue. #913

hrasoa opened this issue Jan 2, 2022 · 14 comments

Comments

@hrasoa
Copy link

hrasoa commented Jan 2, 2022

Bug report

Describe the bug

I'm working on monorepo project which shares components between 3 packages, and sometimes I have some css ordering issues. I made a simple demo in stack blitz which replicates a simplified version of my project:

2 packages that has their own themes:

  • components/app
  • components/ui

SSR made in pages/_document.js

To Reproduce

https://stackblitz.com/edit/nextjs-dzxyti?file=pages/index.js

// components/ui/Button.js
import { styled } from './stitches';

export const Button = styled('button', { textDecoration: 'underline' });
// components/ui/ButtonExtended.js
import { styled } from './stitches';
import { Button } from './Button';

export const ButtonExtended = styled(Button, { textDecoration: 'none' });

Expected behavior

ButtonExtended should have text decoration = none.



In my project

I also tried to concatenate all the getText into one style tag, but still got the issue. And its happens randomly, on the same component sometimes I got the issue, sometimes I don't.

const Label = styled('label', {
  alignItems: 'center',
  borderRadius: '$2xl',
  color: '$accent',
  display: 'flex',
  height: '100%',
  justifyContent: 'center',
  width: '100%',
  variants: {
    checked: {
      true: {
        backgroundColor: '$accent',
        color: '#fff',
      },
    },
  },
});

Resulting markup of with the variant checked true:

<label class="olympos-c-hItJky olympos-c-hItJky-gwBWcw-checked-true" />

Resulting css order:

.olympos-c-hItJky {
    align-items: center;
    border-radius: var(--olympos-radii-2xl);
    color: var(--olympos-colors-accent);
    display: flex;
    height: 100%;
    justify-content: center;
    width: 100%;
}

.olympos-c-hItJky-gwBWcw-checked-true {
    background-color: var(--olympos-colors-accent);
    color: #fff;
}

As you can see the variant got overriden by the base css.

Screenshot 2022-01-02 at 21 09 09

Note: order from SSR is working fine (when I disable the javascript everything is ok), the hydration messes up the orders.

Here's the css from SSR:

Screenshot 2022-01-02 at 21 08 24

Video where I have the bug in multiple places:
https://www.youtube.com/watch?v=nVgBbH8Kv_E

$ yarn list @stitches/react
└─ @stitches/react@1.2.6
@hrasoa hrasoa changed the title CSS order issue on shared components. CSS order issue. Jan 2, 2022
@WDeenik
Copy link

WDeenik commented Jan 3, 2022

Came here to report this exact problem. I would expect that styling a styled base component (creating a 'child' styled component) would override styling of that base component. However, since both components get just one css class assigned, it is completely random if the styling will get overwritten. It is dependent on the order at which the css rules are read by the browser, which is not consistent.

I would expect one of 2 things to happen:

  1. Stitches makes sure that the rules of the child component will always end up below the rules of the base component in the generated styles. This might be an issue with JS execution order and everything.
  2. Stitches makes sure that the child component gets both the classname of the base component as well as an additional classname, and the rules specific for the child component are applied for the combination of both classnames, increasing their specificity.

I'm not familiar with the inner workings of Stitches so I don't know which is the more pragmatic/logical solution here.

@jahirfiquitiva
Copy link

Same issue here 😕

@GeeWizWow
Copy link

Also observing this issue :(

@hadihallak, seems pretty similar to #671, any ideas?

@hadihallak
Copy link
Member

@GeeWizWow There area a few things at work in here like different components extending the same one and two stitches instances at the same time (which is something not currently something we encourage or test against)
Regardless, will be looking into this. thanks for the ping 🙏

@GeeWizWow
Copy link

GeeWizWow commented Feb 14, 2022

Thanks for the reply!

which is something not currently something we encourage or test against

This is interesting, and removing the second instance of stitches does indeed resolve the issue, as seen here: https://stackblitz.com/edit/nextjs-szzpjc?file=pages%2Findex.js

Would you be able to recommend an alternative? We have setup multiple stitches instances as a way to support multiple brands with distinct styling requirements.

For example, would the following be a better approach?

// packages/primitives/theme
export const { createTheme } = createStitches({ ...empty...  });

// packages/brand-one/theme
import { createTheme } from '@primitives/theme';

export const { ... } = createTheme({ ... });

// packages/brand-two/theme
import { createTheme } from '@primitives/theme';

export const { ... } = createTheme({ ... });

@hadihallak
Copy link
Member

Thanks for the reply!

which is something not currently something we encourage or test against

This is interesting, and removing the second instance of stitches does indeed resolve the issue, as seen here: https://stackblitz.com/edit/nextjs-szzpjc?file=pages%2Findex.js

Would you be able to recommend an alternative? We have setup multiple stitches instances as a way to support multiple brands with distinct styling requirements.

For example, would the following be a better approach?

// packages/primitives/theme
export const { createTheme } = createStitches({ ...empty...  });

// packages/brand-one/theme
import { createTheme } from '@primitives/theme';

export const { ... } = createTheme({ ... });

// packages/brand-two/theme
import { createTheme } from '@primitives/theme';

export const { ... } = createTheme({ ... });

Awesome. thanks for the quick followup
Yes this is exactly how I would handle it


I'm curios now if @jahirfiquitiva and @WDeenik are using a similar setups with multiple Stitches instances

@jahirfiquitiva
Copy link

@WDeenik
Copy link

WDeenik commented Feb 15, 2022

We always had just the one Stitches instance, but I am not sure if we used css({}) at the time we encountered this bug. Will check if that was the case around that time.

We don't use css({}) anymore at the moment, so if that was the cause of this bug it might work like expected now, but it will take a while for me to be sure since the bug itself was not present 100% of the time even when we encountered it, and it will involve quite the refactor now.

@jahirfiquitiva Your codesandbox gives an Internal Server Error when I open it

@WDeenik
Copy link

WDeenik commented Feb 15, 2022

I checked when we first encountered this problem, that was right before the release of 1.2.6 which had a fix for a very similar problem. When I ran into it again later and responded to this issue I wasn't aware of that new version yet, so there's a chance that the issue was fixed there. We only had one Stitches instance and did not use css({}).

I cannot reproduce the original bug anymore, but then again it was hard to reproduce back then as well as it only happened occasionally and not very consistent. We'll start extending styled components again (completely avoided it after encountering the bug a couple of times), will report back if we ever run into it again.

@lesha1201
Copy link

I have a problem with CSS order when trying to overwrite styles of styled component using css function. Here is the reproduction with description. I had this problem before 1.2.6 as well (before fixing #671).

@GeeWizWow
Copy link

Would you be able to recommend an alternative? We have setup multiple stitches instances as a way to support multiple brands with distinct styling requirements.
For example, would the following be a better approach?

// packages/primitives/theme
export const { createTheme } = createStitches({ ...empty...  });

// packages/brand-one/theme
import { createTheme } from '@primitives/theme';

export const { ... } = createTheme({ ... });

// packages/brand-two/theme
import { createTheme } from '@primitives/theme';

export const { ... } = createTheme({ ... });

Awesome. thanks for the quick followup Yes this is exactly how I would handle it

Hey @hadihallak, we had a play around with the above this morning, and I'm afraid it wasn't quite what we were looking for, mostly my fault for not having been clear in my examples.

Taking the above code again, what we were really hoping to achieve was something along the lines of:

// packages/primitives/theme
export const { createTheme } = createStitches({
    prefix: '_primitives',
    theme: {
        // nothing here
    },
});

// packages/brand-one/theme
import { createTheme } from '@primitives/theme';

export const { styled } = createTheme({
    colors: {
        red: 'red',
        purple: 'purple',
    },
    media: {
        // some BPs here
    },
    utils: {
        // some utils here,
    },
});

// packages/brand-two/theme
import { createTheme } from '@primitives/theme';

export const { styled } = createTheme({
    colors: {
        green: 'green',
    },
    media: {
        // some **different** BPs here
    },
});

Notice how both brands export an instance of the styled function, looking at the documentation, it appears that the only real way to support this would be to have multiple top level instances of createStitches, but as discussed, than can create issues

@dylanklohr
Copy link

two stitches instances at the same time (which is something not currently something we encourage or test against) Regardless, will be looking into this. thanks for the ping 🙏

Hey @hadihallak, this is actually something that I've brought up before in #534, for which the fix was #546.

When the change to using CSSOM was put out, it's something that I explicitly checked with @jonathantneal about, where we had the following conversation


DYLAN: does this also remove the ability for consumers to specify the id of the style element that their styles are inserted into, and by default they would all be written to <style id="stitches" /> ?

JON: There is no longer a required ID, as there was before. However, we plan to add some of the other abilities for you to target the stylesheet or where it is inserted.
The big reason for that particular change is to deal with the fact that Stitches no longer overrides the whole style sheet all at once. So there’s no “callback” for the big ol’ update.
For now, Stitches figures out which stylesheet it belongs to.

DYLAN: Interesting. Would I be correct in assuming then that multiple copies of stitches running on the same page then (each with their unique prefix) would now be compatible running on the same page without potentially overriding styles from another instance?

JON: Yes. That was actually one of the use-cases. Stitches Core running alongside Stitches React, or doubling one of those.


Hopefully the context is helpful! 😄

@rista404
Copy link

Hey everyone, the original description of the bug perfectly explains the bugs I'm having now on a new project with Hydrogen framework. Basically the overrides and variants have the same specificity in the final css but are before the original (base) class and therefore they don't get applied. It's stitches version 1.2.8

I could just use !important everywhere but I think this should be fixed on the library level. I understand it is not a minor bug and it needs time but can somebody at least tell us are there any plans to fix this and when?

Aside from this the library is great and I love it, thanks! 🙏🏻

@linuz90
Copy link

linuz90 commented Oct 20, 2022

Also having this issue now:
CleanShot 2022-10-20 at 14 43 34@2x

Oddly enough, the static server-side rendered css seems correct, but if I navigate to the page I'm working on, then the inline css is not correctly overriding the base styled css.

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

No branches or pull requests

9 participants