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

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

Open
MaciejZajac opened this issue Apr 29, 2024 · 7 comments
Open

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

MaciejZajac opened this issue Apr 29, 2024 · 7 comments

Comments

@MaciejZajac
Copy link

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
postcss/postcss-nested#141

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

@elchininet
Copy link
Owner

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
Copy link
Owner

elchininet commented Apr 30, 2024

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.

@elchininet elchininet reopened this Apr 30, 2024
@louy
Copy link

louy commented Apr 30, 2024

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
Copy link
Owner

elchininet commented Apr 30, 2024

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
Copy link
Owner

elchininet commented Apr 30, 2024

@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
Copy link

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
Copy link
Owner

elchininet commented May 2, 2024

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

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

No branches or pull requests

4 participants