elchininet / postcss-rtlcss

PostCSS plugin to automatically build Cascading Style Sheets (CSS) with Left-To-Right (LTR) and Right-To-Left (RTL) rules using RTLCSS
https://elchininet.github.io/postcss-rtlcss/
Apache License 2.0
96 stars 14 forks source link

Plugin does not work with media queries nested inside selectors #324

Open MaciejZajac opened 2 months ago

MaciejZajac commented 2 months ago

Plugin doesn't seem to add a prefix when there is a media query nested in a selector .

.self {
    padding: 10px 10px 14px;
}

.self {
    @media (width > 1024px) {    
        padding: 10px 10px 12px;
    }
}

expected:

[dir] .self {
    @media (--bp-laptop) {    
        padding: var(--spacing-1000) var(--spacing-400) 120px;
    }
}

[dir] .self {
    padding: var(--spacing-1000) var(--spacing-400) 120px;
}

actual:

.self {
    @media (--bp-laptop) {    
        padding: var(--spacing-1000) var(--spacing-400) 120px;
    }
}

[dir] .self {
    padding: var(--spacing-1000) var(--spacing-400) 120px;
}

According to this nested media is a valid css https://github.com/postcss/postcss-nested/issues/141

Playground link https://elchininet.github.io/postcss-rtlcss/#662f7595f2033

elchininet commented 2 months ago

Hi @MaciejZajac,

That is correct, putting a media-query containing only declarations inside a rule is a valid SASS code. But even if this plugin has a basic support for rules inside media-queries it does not do the entire job that Dart-sass does, that is not the purpose of this plugin and it will never be.

PostCSS-RTLCSS (as well as RTLCCS) expects that you provide an already compiled CSS input, not a native SASS code, so my recommendation is that you run Dart-sass or any other plugin that is created to compile SASS code into CSS before running PostCSS-RTLCSS and not in the other way around.

Regards

elchininet commented 2 months ago

Hi @MaciejZajac, I am seeing that since December 2023, CSS nesting is supported by the major browsers. I'll reopen the issue but do not expect that this will be solved in a near future, my recommendation of using some tool to unwrap the nesting remains.

image.

louy commented 2 months ago

Thanks @elchininet Yes this is standard CSS syntax and not SASS as mentioned by @MaciejZajac

Would you accept contributions to fix it? And can you give pointers on where this could be fixed in the codebase if it was to be fixed? I'd love to assess effort

elchininet commented 2 months ago

Thanks @louy,

I knew that the proposal was ongoing but I didn't know that the support right now is above 85% and it has become a new baseline since December.

Contributions are always welcomed, but implementing it would be a bit hard to achieve, just because the way in which the plugin works at the moment. It expects that inside an at-rule could be rules or other at-rules, but not declarations. If it the declarations parser is run inside the at-rules, then the parser itself needs to be refactored to allow this togeteher with all the store logic that expects to contain rules that needs to be flipped because their declarations were flipped (in this case they should be rules that should be flipped because any at-rule at any level inside it contains declarations that have been flipped). All of this should be done taking into account all the logic that this plugin provides to deal with overriden properties and avoid a rule being overriden because specificity of prefixed rules, in this case it should do the same but taking into account any at-rule nested inside a rule.

If you come with an easy solution to deal with this, I would be happy to discusss it and help you to get it implemented.

Regards

elchininet commented 2 months ago

@louy, Just take into account, as I posted before, that at the moment the plugin does support basic nesting if the at-rules contain rules instead of declarations. This is the same code, but placing the rule inside the at-rule instead of in the other way around, and as you can check it works. What doesn't work at the moment is to have declarations inside an at-rule because what should be flipped in those cases is the nearest rule above the tree (or the rules above that one if there was some overriding).

mjuniquecode commented 2 months ago

Hello @elchininet so do i get this correctly plugin will strugle with css that has some simple media queries ? Is there a configuration that will go inside this nested @media and add [dir] attribute there ?

Example: `.navigation__item { padding-left: 50px; padding-right: 40px }

@media (min-width: 960px) { .navigation__item { padding: 8px 12px; border-radius: 8px }`

is beeing transformed to:

`[dir="ltr"] .navigation__item { padding-left: 50px; padding-right: 40px }

[dir="rtl"] .navigation__item { padding-right: 50px; padding-left: 40px }

@media (min-width: 960px) { .navigation__item { padding: 8px 12px; border-radius: 8px }`

And this new code creates an issue with [dir="rtl"] .navigation__item beeing "stronger" selector than @media causing layout to break.

elchininet commented 2 months ago

Hi @mjuniquecode, The plugin only struggles to manage media-queries that contain declarations inside (something that has been included in the baseline just some months ago). For media-queries that contain rules, as in your example, you just need to use the safeBothPrefix option if you want to add the bothPrefix to those rules that could potentially be overridden by prefixed rules due to higher specificity.

https://elchininet.github.io/postcss-rtlcss/#66339404861da