Treat pseudo selectors like emotion / styled-components?

WorldSEnder commented 3 years ago

The currently implemented way to emit a block with a qualifier according to https://github.com/futursolo/stylist-rs/blob/c59568c943deb0500397915f4d462ac5aa140b03/packages/stylist-core/src/ast.rs#L127-L137 is to prefix that with the injected class name. I believe an erroneous additional whitespace was being added already in css-in-rust. To show a few examples:

.extra { background: red; }
:not(body) { background: red; }
&:not(body) { background: red; }

becomes

.stylist-1 .extra { background: red; }
.stylist-2 :not(body) { background: red; }
//        ^ erroneous space, this is now a descendant combinator!
.stylist-3:not(body) { background: red; }
//        ^ all good!

I would propose a more radical change while fixing the above: simply do not inject the class when no '&' is present in the condition. Just replace all '&' and leave the rest be in blocks. So the above would become

.extra { background: red; }
// Now a global style
:not(body) { background: red; }
//  A breaking change but actually consistent with what material-ui does :)
.stylist-3:not(body) { background: red; }
//        ^ all good!

I believe the proposed syntax handling is more consistent with the handling in material-ui where that '&' seems to be required or at least commonly used, see for example https://github.com/mui-org/material-ui/blob/49b75b08a6584c930fdd23a0eb1fe28f6e1ffce5/packages/material-ui/src/Button/Button.js#L229-L241

This would also readdress #4 and give an escape-hatch for components that for some reason can't specify a class for an element, e.g. because they use a third-party js library that render the components.

futursolo commented 3 years ago

As CSS-In-Rust cannot parse your example, I have modified it to the example below:

background: red;
:not(body) { background: red; }
&:not(body) { background: red; }
.extra { background: red; }

Under master branch, css-in-rust generates the following stylesheet:

.CustomComponent-4597592370089325376 {
background: red;
}
.CustomComponent-4597592370089325376 :not(body)  {
                                    /* space here */
background: red;
}
 .CustomComponent-4597592370089325376:not(body)  {
background: red;
}
.CustomComponent-4597592370089325376 .extra  {
background: red;
}

Stylist generated stylesheet:

.stylist-Rh02X9kD {
background: red;
}
.stylist-Rh02X9kD :not(body) {
background: red;
}
.stylist-Rh02X9kD:not(body) {
background: red;
}
.stylist-Rh02X9kD .extra {
background: red;
}

The behaviour matches.

However, this behaviour do not match emotion or styled-components.

They generated something like:

.hYXQpd{
background:red;
}
.hYXQpd:not(body){
background:red;
}
.hYXQpd:not(body){
background:red;
}
.hYXQpd .extra{
background:red;
}

In addition, material-ui v5 is going to adopt emotion and drop their own css solution. So this will also become the behaviour of material-ui.

So the question becomes whether to treat pseudo selectors like styled-components or emotion.

As for global styles, I think the proper way to deal with it is like emotion and styled-components, introduce a <Global /> component for global styles and otherwise all scoped.

IMO, the main advantage of a css-in-rust / css-in-js solution over the traditional CSS / SCSS / LESS solution is to have scoped css. Not requiring it opens a door for people to write global css everywhere (sometimes without noticing it).

Which really defeats the purpose of such a solution.

In short, You should use <GlobalStyle /> to apply global styles, or add your class name to <html /> manually.

WorldSEnder commented 3 years ago

Thanks for doing the digging! I concur that it should match emotion behavior where documented and intended. When reading through their docs, they always use the ampersand. If only there was documentation available with strict rules instead of example after example. I don't want to just match their output if that might be coincidental and not intended.

E.g: I don't understand

.hYXQpd .extra{
    /* ^ why the extra space here but not for pseudo selectors? */
    /* seems to fall into the same trap of now being a descendant combinator */
background:red;
}

I'd rather fail or at least warn about potentially misleading output than exactly match some codified but not written spec.

Apart from that, remaining issue then for me is only that extra space, completely on your side with Global!

WorldSEnder commented 3 years ago

I found https://github.com/emotion-js/emotion/issues/235#issuecomment-320438237 that claims that the & is a requirement from v7 onwards, contradicting observed behavior...

futursolo / stylist-rs

Treat pseudo selectors like emotion / styled-components? #16