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

postcss-logical changes selector specificity #90

Closed
mi5ha6in opened this issue Dec 24, 2021 · 9 comments · Fixed by #472
Closed

postcss-logical changes selector specificity #90

mi5ha6in opened this issue Dec 24, 2021 · 9 comments · Fixed by #472
Labels
breaking This is a breaking change plugins/postcss-logical

Comments

@mi5ha6in
Copy link

I use it together with the postcss-dir-pseudo-class. A problem arises when you need to change the value to the center in media queries, for example, text-align: start to text-align: center

.selector {
  text-align: start;
}

@media (max-width: 600px) {
  .selector {
   text-align: center;
  }
}

Generated:

[dir=‘ltr’] .selector { text-align: left }

@media (max-width: 600px) {
  .selector { text-align: center; }
}

And since the first selector has more weight, the media doesn't work.

@mi5ha6in mi5ha6in changed the title using center meanings in media expressions use of center values in media expressions Dec 24, 2021
@romainmenke romainmenke changed the title use of center values in media expressions postcss-logical changes selector specificity Dec 24, 2021
@romainmenke
Copy link
Member

Thank you for reporting this @mi5ha6in

I think the same technique as mentioned here can be used in postcss-logical to preserve selector specificity : #88

We will look into this!


Also wanted to ask if there was a specific reason not to use one the issue templates?
Nothing wrong with not using a template!

Just want to check if there is something we can improve here?

@mi5ha6in
Copy link
Author

Also wanted to ask if there was a specific reason not to use one the issue templates? Nothing wrong with not using a template!

Just want to check if there is something we can improve here?

It seemed to me that not one of the issue templates does not fit the situation. If you look at what the plugin is supposed to do, it does. It doesn't do folbeck for "center" values because they are supported. But that's what caused the conflict with those properties for which the folkback is done. In general, I was unable to define a category for the template:)

@romainmenke
Copy link
Member

It seemed to me that not one of the issue templates does not fit the situation. If you look at what the plugin is supposed to do, it does. It doesn't do folbeck for "center" values because they are supported. But that's what caused the conflict with those properties for which the folkback is done. In general, I was unable to define a category for the template:)

Screenshot 2021-12-24 at 18 13 46

This is the issue template we intended to be used for cases like this as you are reporting that the output CSS is not ideal.

Would it be clearer if we change the wording like this :

- CSS output isn't what you expected? This is your path!
+ CSS output is unexpected or breaks in certain cases? This is your path!

@mi5ha6in
Copy link
Author

Would it be clearer if we change the wording like this :

- CSS output isn't what you expected? This is your path!
+ CSS output is unexpected or breaks in certain cases? This is your path!

Yes, that would be clearer to me. Thank you very much!

@romainmenke
Copy link
Member

@mi5ha6in I have updated the issue template description.


back on topic

I have a proposal ready here : #91
But I fear it is putting too much limitations in terms of browsers we can support with it.

Another possible fix could be to also detect values like center and transform them.

Input :

.selector {
  text-align: start;
}

@media (max-width: 600px) {
  .selector {
   text-align: center;
  }
}

Output :

[dir="ltr"] .selector {
  text-align: start;
}

@media (max-width: 600px) {
  [dir="ltr"] .selector {
   text-align: center;
  }

  [dir="rtl"] .selector {
   text-align: center;
  }
}

The rules with values like center would need to be inserted twice.
Once with ltr and once with rtl. This way we match selector specificity.

I am hesitant about this approach too as I don't have a good sense of what properties and values would need a fix like this. Considering shorthand and long form properties this can easily get out of hand.


Because there is no straightforward fix I am afraid it might take a while to get fixed.

Can you share the plugin options you use for postcss-logical?
It might be possible to work around this for now with a different setup.

@mi5ha6in
Copy link
Author

mi5ha6in commented Dec 26, 2021

I have a proposal ready here : #91

It seems that for me this will solve the problem.

Can you share the plugin options you use for postcss-logical? It might be possible to work around this for now with a different setup.

I was doing just a test on a project without additional settings. The goal was to support logical values in Safari 13

.pipe(postcss([
    postcssLogical()
  ]))
.pipe(postcss([
    postcssDirPseudoClass()
  ]))

@romainmenke
Copy link
Member

romainmenke commented Dec 26, 2021

Is it possible for you to switch CSS bundle through server side logic?

For example :

with some handle bars like template to illustrate

<!DOCTYPE html>
<html
	{% if dir == "rtl" %}
		dir="rtl"
	{% else %}
		dir="ltr"
	{% endif %}
>
<head>
	{% if dir == "rtl" %}
		<link rel="stylesheet" href="style.rtl.css">
	{% else %}
		<link rel="stylesheet" href="style.ltr.css">
	{% endif %}
</head>
<body>
	...
</body>
</html>

In your gulp config you then need to add a second output to get two outputs.

.pipe(postcss([
    postcssLogical({ dir: 'ltr' })
  ]))
.pipe(gulp.dest('style.ltr.css'));
.pipe(postcss([
    postcssLogical({ dir: 'rtl' })
  ]))
.pipe(gulp.dest('style.rtl.css'));

When the dir option is used no :dir pseudo classes will be added to the output.
So the selector specificity won't change.

If however you only need ltr or rtl at the moment but still want to use logical properties and values you can simply keep the single output and use :

.pipe(postcss([
    postcssLogical({ dir: 'ltr' })
  ]))

// or

.pipe(postcss([
    postcssLogical({ dir: 'rtl' })
  ]))

This obviously is not a fix, but it might help you out until we can find a good solution.


side note :

you can get a faster build by bundling the plugins

.pipe(postcss([
    postcssLogical(),
    postcssDirPseudoClass()
  ]))

@mi5ha6in
Copy link
Author

Is it possible for you to switch CSS bundle through server side logic?

Yes, we came to a similar one. Thank you for your help

you can get a faster build by bundling the plugins

Exactly!

@Antonio-Laguna Antonio-Laguna added the breaking This is a breaking change label Apr 10, 2022
@romainmenke romainmenke linked a pull request Jan 23, 2023 that will close this issue
@romainmenke
Copy link
Member

Hi @mi5ha6in,
We have released a new version of postcss-logical that works more like a fallback and doesn't try to be a full polyfill.

https://www.npmjs.com/package/postcss-logical

This new version inserts fallback properties and values but doesn't generate extra rules with :dir() and increased specificity.

Thank you gain for reporting this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking This is a breaking change plugins/postcss-logical
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants