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

The plugin does not honour the stringMap directive. #242

Closed
3baaady07 opened this issue Dec 27, 2023 · 14 comments · Fixed by #244
Closed

The plugin does not honour the stringMap directive. #242

3baaady07 opened this issue Dec 27, 2023 · 14 comments · Fixed by #244

Comments

@3baaady07
Copy link

As can be seen from the illustration, the string highlighted is the same after being processed, and the selector as well is not changed.
image

before

/* rtl:begin:options: {
  "stringMap":[ {
    "name"    : "prev-next",
    "search"  : "prev",
    "replace" : "next"
  } ]
} */
.carousel-control-prev-icon {
  background-image: url("data:image/svg+xml,%3csvg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 16 16' fill='%23fff'%3e%3cpath d='M11.354 1.646a.5.5 0 0 1 0 .708L5.707 8l5.647 5.646a.5.5 0 0 1-.708.708l-6-6a.5.5 0 0 1 0-.708l6-6a.5.5 0 0 1 .708 0z'/%3e%3c/svg%3e");
}

.carousel-control-next-icon {
  background-image: url("data:image/svg+xml,%3csvg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 16 16' fill='%23fff'%3e%3cpath d='M4.646 1.646a.5.5 0 0 1 .708 0l6 6a.5.5 0 0 1 0 .708l-6 6a.5.5 0 0 1-.708-.708L10.293 8 4.646 2.354a.5.5 0 0 1 0-.708z'/%3e%3c/svg%3e");
}
/* rtl:end:options */

After

/* rtl:begin:options: {
  "stringMap":[ {
    "name"    : "prev-next",
    "search"  : "prev",
    "replace" : "next"
  } ]
} */
.carousel-control-prev-icon {
  background-image: url("data:image/svg+xml,%3csvg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 16 16' fill='%23fff'%3e%3cpath d='M11.354 1.646a.5.5 0 0 1 0 .708L5.707 8l5.647 5.646a.5.5 0 0 1-.708.708l-6-6a.5.5 0 0 1 0-.708l6-6a.5.5 0 0 1 .708 0z'/%3e%3c/svg%3e");
}

.carousel-control-next-icon {
  background-image: url("data:image/svg+xml,%3csvg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 16 16' fill='%23fff'%3e%3cpath d='M4.646 1.646a.5.5 0 0 1 .708 0l6 6a.5.5 0 0 1 0 .708l-6 6a.5.5 0 0 1-.708-.708L10.293 8 4.646 2.354a.5.5 0 0 1 0-.708z'/%3e%3c/svg%3e");
}
/* rtl:end:options */
@elchininet
Copy link
Owner

elchininet commented Dec 27, 2023

Hi @3baaady07,

postcss-rtlcss does not have rtl:options directives, these are the directives supported by this plugin. I think that you are getting that directive from rtlcss but that is only valid if you use rtlcss directly.

To use a stringMap with postcss-rtlcss, you need to use it through an option, and you will need to use also the autoRename option together to flip the selectors names (by default it is disabled).

Here you have a working example with your CSS code:

https://runkit.com/embed/wxaht3xnjabj

Note: postcss-rtlcss (as well as rtlcss) will not flip the SVG that you are using as background-image, so, do not expect any changes in those strings.

@3baaady07
Copy link
Author

3baaady07 commented Dec 27, 2023

Hi @elchininet.
I'd like to thank you for the work you done on this plugin.
But for some reason, this particular option is not working in my Vite project. Here's the code in postcss.config.js

import postcssRTLCSS from 'postcss-rtlcss';
import { Autorename } from 'postcss-rtlcss/options';

export default {
   plugins: {
      autoprefixer: {},
      ['postcss-rtlcss']: postcssRTLCSS({
         autoRename: Autorename.strict,
         stringMap: [
            {
               name: 'prev-next',
               search: 'prev',
               replace: 'next'
            }
         ]
      })
   },
}

Maybe this would shed light on where exactly I went wrong?

Another note is that based on the runkit example you provided, the plugin does not generate two different selectors prefixed with ([dir=ltr] and [dir=rtl]). Is that an intended behaviour or is there an extra step I should perform?

@elchininet
Copy link
Owner

elchininet commented Dec 28, 2023

Hi @3baaady07,

But for some reason, this particular option is not working in my Vite project. Here's the code in postcss.config.js

Is postcss-rtlcss working in your project and only that option is not working for you, or is postcss-rtlcss nor working at all?

Another note is that based on the runkit example you provided, the plugin does not generate two different selectors prefixed with ([dir=ltr] and [dir=rtl]). Is that an intended behaviour or is there an extra step I should perform?

postcss-rtlcss adds prefixed rules when the content of the rule changes, it was built having something like this in mind. Your case it is an interesting one because the content of the rule doesn‘t change (as it is an SVG there is no way to detect the direction by its name as with an image), your only workaround for now would be doing something like this, but I will try to think what could be the best way to solve your use case without affecting the previous functionality. I‘ll let this issue open until I find an answer for that.

@3baaady07
Copy link
Author

Hi @elchininet,

Is postcss-rtlcss working in your project and only that option is not working for you, or is postcss-rtlcss nor working at all?

The plugin works as expected (prepending the html attribute dir to selectors with the appropriate values). The stringMap, however, does not have an effect even with the added postcss configuration.

...your only workaround for now would be doing something like this, but I will try to...

The SVG values come from sass variables. Those variables are processed by the a third party sass function (Bootstrap - escape-svg) that has a specific case for url(). When adding the rtl directive with a url value, the function distorts the resultent value rendering faulty css.

I'll open an issue with Bootstrap about this but I have more faith in this plugin.. 😄

@elchininet
Copy link
Owner

elchininet commented Dec 29, 2023

Hi @3baaady07,

I would need a minimum reproducible example in a repo to debug it, otherwise it is very difficult to know what could be happening. Just make sure that those rules do not contain directional properties, stringMap will affect rules without directional properties, if the rules are processed in any way creating prefixed rules, stringMap will not do anything in them.

About adding the directives in SASS, just make sure that you are adding them in this way.

@3baaady07
Copy link
Author

Hi @elchininet,

I have added a basic repo that illustrates the issue. The detailed reproducible steps are described in the README.md.

Cheers, :)

@elchininet
Copy link
Owner

elchininet commented Dec 29, 2023

Hi @3baaady07,
Thanks for the repo, it is the best way to debug something.
What happens is that the postcss.config.js file is not using a valid configuration. I have consulted the Vite documentation and they mention that the format of the config can be any supported format of postcss-load-config. So, change your configuration to any of these ones and it will work:

Plugins as an object

import { Autorename } from 'postcss-rtlcss/options';

export default {
   plugins: {
      'postcss-rtlcss': {
         autoRename: Autorename.strict,
         stringMap: [
            {
               name: 'prev-next',
               search: 'prev',
               replace: 'next'
            }
         ]
      }
   }
}

Plugins as an array

import postcssRTLCSS from 'postcss-rtlcss';
import { Autorename } from 'postcss-rtlcss/options';

export default {
   plugins: [
      postcssRTLCSS({
         autoRename: Autorename.strict,
         stringMap: [
            {
               name: 'prev-next',
               search: 'prev',
               replace: 'next'
            }
         ]
      })
   ]
}

@elchininet
Copy link
Owner

Hi @3baaady07,

Check this pull request, read the statement and let me know if it is clear and would solve your use case.

@3baaady07
Copy link
Author

hi @elchininet,

Plugins as an object

import { Autorename } from 'postcss-rtlcss/options';

export default {
   plugins: {
      'postcss-rtlcss': {
         autoRename: Autorename.strict,
         stringMap: [
            {
               name: 'prev-next',
               search: 'prev',
               replace: 'next'
            }
         ]
      }
   }
}

That actually fixed it 👍
However, the desired result is to generate two different selectors prefixed with [dir=***] attribute which your recent pull request solves. My only concern is that I'm not too sure if I'm able to create bounds to the effect of processRuleNames.

For example:

/* I want to change those */
.test2-right::before {
  content: "\f007";
}

.test2-left::before {
  content: "\f010";
}

/* ...but not those */
.hand-right {
  background-image: url("images/hand-right-[hash].png");
}

.hand-left{
  background-image: url("images/hand-left-[different hash].png");
}

Sorry if this sounds like a convoluted request.

@elchininet
Copy link
Owner

elchininet commented Dec 30, 2023

Hi @3baaady07,

You can use the rtl:ignore directives for that:

/* I want to change those */
.test2-right::before {
  content: "\f007";
}

.test2-left::before {
  content: "\f010";
}

/* ...but not those */
/*rtl:begin:ignore*/
.hand-right {
  background-image: url("images/hand-right-[hash].png");
}

.hand-left{
  background-image: url("images/hand-left-[different hash].png");
}
/*rtl:end:ignore*/

But just in case that you want to ignore it in the whole file and just apply it to some rules, I have plans of creating a directive for that once the new version is released.

But remember that you can use what I recomended you here with the current version of the library without setting any option:

https://www.sassmeister.com/gist/9a7c50edd00fa7ae68616b711816e8b5

@3baaady07
Copy link
Author

https://www.sassmeister.com/gist/9a7c50edd00fa7ae68616b711816e8b5

This is the exact solution I proposed to Bootstrap in this PR. Hopefully they will merge it because the library is downloaded as part of the CD pipeline

Thank you @elchininet. Cheers.

@elchininet
Copy link
Owner

Hi @3baaady07,

postcss-rtlcss will do the same regardless the change in that pull request. The change that you are proposing will change the values instead of changing the rule names but this will not affect the way postcss-rtlcss manage the resulting file.

Result before the pull request:

/* Input */
.carousel-control-prev-icon {
   // prev-content
}
.carousel-control-next-icon {
   // next-content
}

/* Output */
.carousel-control-next-icon {
   // prev-content
}
.carousel-control-prev-icon {
   // next-content
}

Result after the pull request:

/* Input */
.carousel-control-prev-icon {
   // prev-content
}
.carousel-control-next-icon {
   // next-content
}

/* Output */
.carousel-control-prev-icon {
   // next-content
}
.carousel-control-next-icon {
   // prev-content
}

By the way, after this pull request is merged and released, you will have the ability of applying the processRuleNames option more granular.

@3baaady07
Copy link
Author

Hi @elchininet ,

postcss-rtlcss will do the same regardless the change in that pull request. The change that you are proposing will change the values instead of changing the rule names but this will not affect the way postcss-rtlcss manage the resulting file.

I believe the resulting file will be different with the proposed PR. You illustrated this in your runkit example.

The idea is as follows...

/* Input */
.carousel-control-prev-icon {
   // prev-content /* rtl:{next-content} */
}
.carousel-control-next-icon {
   // next-content /* rtl:{prev-content} */
}

/* Output */
[dir=ltr] .carousel-control-prev-icon {
   // prev-content
}
[dir=ltr] .carousel-control-next-icon {
   // next-content
}
[dir=rlt] .carousel-control-prev-icon {
   // next-content
}
[dir=rlt] .carousel-control-next-icon {
   // prev-content
}

Am I missing something?

@elchininet
Copy link
Owner

@3baaady07,

It would work if you use the bootstrap uncompiled source file, but if you use the CSS files that bootstrap provides, they will not have the directives there, so you will get the same result but just in a different order.

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

Successfully merging a pull request may close this issue.

2 participants