Skip to content

Conversation

@aleen42
Copy link
Contributor

@aleen42 aleen42 commented May 9, 2024

This pull request implements an option named processDeclarationPlugins to support stepping into the process of processing CSS declarations, like avoiding flipping background-positions.

For instance, RTLCSS will flip the background-position:

input

.test {
    background-position: 0 100%;
}
Convert 0 to 100% (default)

output

.test {
    background-position: 100% 100%;
}

However, we may need to stop this unexpected flipping in the case of using it to clip Sprite images, so we can use this introduced option to avoid this:

const options = {
	processDeclarationPlugins: [
        {
            name: 'avoid-flipping-background',
            priority: 99, // above the core RTLCSS plugin which has a priority value of 100
            processors: [{
                expr: /(background|object)(-position(-x)?|-image)?$/i,
                action: (prop, value) => ({prop, value})}
            ]
        }
    ]
};
output
.test {
    background-position: 0 100%;
}

@aleen42 aleen42 force-pushed the master branch 4 times, most recently from 69fdeab to 49e38d2 Compare May 9, 2024 06:54
@elchininet
Copy link
Owner

elchininet commented May 9, 2024

Hi @aleen42,

It is always recommended to discuss the intention of creating a feature before working on it, just to avoid working in vain.

If you want to support plugins and hooks, it cannot be done in that way that you did and you cannot link plugins and hooks of RTLCSS. It will create confussion and it will be a source of issues because most of the things supported by RTLCSS will not work with this plugin.

postcss-rtlcss uses RTLCSS behind the scenes, but this does not mean that it runs RTLCSS in the entire CSS input. RTLCSS is used only on declarations, but it doesn‘t run in any other element (the CSS root element, the rules, at rules, comments, etc). This means that anything that a user sets inside directives will potentially fail, I think that the only thing that will work without issues is the processors property.

The same with hooks, as RTLCSS is not executed in the CSS root, most probably what RTLCSS users expect from a hook will not work with postcss-rtlcss. So, if you want to build hooks, most probably it should be built here from scratch (the same way the control directives and the stringMap option are built from scratch).

Please, refactor this following the next:

  • Do not use the types from RTLCSS, create your own types. You need to test what works with the plugin and the things that don‘t work should be removed (most probably the only thing that will work will be the processors property).
  • Do not link to the RTLCSS options because as the options will not work in the same way, it will create confussion.
  • Create tests for any piece of code that you introduce.
  • Add a description to the pull request, explaining the objectives of the pull request as well as some examples. This is helpful to keep track of the changes introduced. (take this or this as an example)

Regards and thanks for contributing to the project.

@coveralls
Copy link

coveralls commented May 9, 2024

Coverage Status

coverage: 100.0%. remained the same
when pulling dd78fd0 on aleen42:master
into d220b64 on elchininet:master.

@aleen42
Copy link
Contributor Author

aleen42 commented May 10, 2024

@elchininet Thanks for your review. I have eliminated the unnecessary hooks options because it seems we do not need to hook during processing CSS declarations via postcss-rtlcss. The plugin options have been renamed as processDeclarationPlugins which are currently only applied during processing declarations.

@aleen42 aleen42 force-pushed the master branch 2 times, most recently from 37df2ae to 82b24c3 Compare May 10, 2024 03:49
@aleen42 aleen42 changed the title Support specifying plugins or hooks when using rtlcss Support specifying processors during processing CSS declarations May 13, 2024
@aleen42 aleen42 requested a review from elchininet May 13, 2024 07:31
@elchininet
Copy link
Owner

Hi @aleen42,
Looks good to me. Update the branch (I think that you may get some merge conflicts).
Regards

@elchininet
Copy link
Owner

@aleen42,
I think that it is ready to merge.
Thanks for your contribution.
I'll release a new version when I have a chance.
Regards 👍🏼

@elchininet elchininet merged commit f6f54cf into elchininet:master May 21, 2024
@elchininet
Copy link
Owner

Hi @aleen42,
The feature has been released in version 5.3.0.
Regards

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 this pull request may close these issues.

3 participants