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

Changed RTL processing of carousel control icons #39536

Merged
merged 5 commits into from
Feb 6, 2024
Merged

Conversation

3baaady07
Copy link
Contributor

@3baaady07 3baaady07 commented Dec 29, 2023

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

Copy link
Member

@julien-deramond julien-deramond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

scss/_carousel.scss Outdated Show resolved Hide resolved
scss/_carousel.scss Outdated Show resolved Hide resolved
@3baaady07
Copy link
Contributor Author

Hi @julien-deramond,

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

Copy link
Member

@julien-deramond julien-deramond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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

Copy link
Member

@louismaximepiton louismaximepiton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

3baaady07 commented Jan 7, 2024

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,

Copy link
Member

@ffoodd ffoodd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

Thanks for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants