Changed RTL processing of carousel control icons

[3baaady07]

Description

Changed the way RTL processes carousel icons (carousel-control-prev-icon and carousel-control-next-icon) . The processing originally uses rtl:options, whereas now it uses an inline rtl:{value} to flip the values instead of changing the selector name.

Motivation & Context

1. This was done to enable tools such as postcss-rtlcss to process the files and generate a file with both [dir=rtl] and [dir=ltr] prefixed selectors.
2. It was also to prevent rtl:options from affecting css that is scoped outside of the intended rtl:options block.

Type of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Refactoring (non-breaking change)
• Breaking change (fix or feature that would change existing functionality)

Checklist

• I have read the contributing guidelines
• My code follows the code style of the project (using npm run lint)
• My change introduces changes to the documentation
• I have updated the documentation accordingly
• I have added tests to cover my changes
• All new and existing tests passed

Live previews

• https://deploy-preview-39536--twbs-bootstrap.netlify.app/docs/5.3/components/carousel/
• https://deploy-preview-39536--twbs-bootstrap.netlify.app/docs/5.3/examples/cheatsheet-rtl/#carousel

[julien-deramond]

Thanks for this PR @3baaady07

I don't mind changing the approach if it is better supported by external tools and if the generated CSS is the same. My knowledge on the topic is rather limited so @ffoodd, if you have a little bit of time to double-check it too :) Maybe there was a specific reason I'm not aware of to use rtl:options 🤷

On the generated aspect. I've compared both bootstrap.rtl.css. Here's the diff:

ju ߷ ~/t/bootstrap ➤ (main) diff dist/css/bootstrap.rtl.css dist/css/bootstrap.new.rtl.css                                                               (0.000002 hrs)
6126,6128d6125
< }
< .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='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");
6132a6130,6133
> }
> 
> .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='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");
6186,6187c6187,6188
< .carousel-dark .carousel-control-next-icon,
< .carousel-dark .carousel-control-prev-icon {
---
> .carousel-dark .carousel-control-prev-icon,
> .carousel-dark .carousel-control-next-icon {
6197,6199c6198,6200
< [data-bs-theme=dark] .carousel .carousel-control-next-icon,
< [data-bs-theme=dark] .carousel .carousel-control-prev-icon, [data-bs-theme=dark].carousel .carousel-control-next-icon,
< [data-bs-theme=dark].carousel .carousel-control-prev-icon {
---
> [data-bs-theme=dark] .carousel .carousel-control-prev-icon,
> [data-bs-theme=dark] .carousel .carousel-control-next-icon, [data-bs-theme=dark].carousel .carousel-control-prev-icon,
> [data-bs-theme=dark].carousel .carousel-control-next-icon {

The only difference relies on an extra break line (which is good) and the order of declarations: .carousel-control-prev-icon and .carousel-control-next-icon are now inverted. But it doesn't change the spirit or the final rendering since there's not especially a rule depending on the other one in terms of priority.

The rendering and user experience are the same as before (see https://deploy-preview-39536--twbs-bootstrap.netlify.app/docs/5.3/examples/cheatsheet-rtl/#carousel).

[3baaady07]

Hi @julien-deramond,

What's the timeline for making a decision regarding this PR?

[julien-deramond]

I've updated the PR to take into account the comments so that it handles correctly the whitespaces. Based on my current knowledge of the topic (see #39536 (review)), I think it's safe to merge.

What's the timeline for making a decision regarding this PR?

@3baaady07 It can take a little bit of time because it'll depend on the availability of the other maintainers in their spare time to confirm that this approach doesn't imply any regressions. But there probably won't be any other actions on your end, either it will be approved and merged as is, or closed if there's an issue with this approach.

On my side, I'll be off for a couple of weeks. I'll check back on the status of this PR when I'll get back.

[3baaady07]

Thank you @julien-deramond,
I'll use patch-package till a decision is made 😊
Cheers 🤙

[louismaximepiton]

Seems easier to maintain indeed, the only drawback I can see here is that the bootstrap.css increases its size by .04kB. So I'd be in favor of this change if it can help people.
Another solution to scope the changes could be to use /* rtl:begin:options */ and /* rtl:end:options */ source

[3baaady07]

Hi @louismaximepiton,

The trade-off to the increase in size is to enable postcss-rtlcss to process the css and generate a single, optimized file with selectors prefixed with [dir=rtl] and [dir=ltr]. postcss-rtlcss is a plugin promoted by the reputable module bundler plugin postcss, however postcss-rtlcss does not process /* rtl:options */ directive or any of its derivatives.

The changes proposed enable the plugin to process the css file while functionally maintaining the rtlcss output.

Cheers,

[ffoodd]

LGTM 🚀

Thanks for the PR!