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

Fix light mode carousel in dark mode #40695

Merged
merged 3 commits into from
Dec 19, 2024

Conversation

julien-deramond
Copy link
Member

@julien-deramond julien-deramond commented Aug 6, 2024

Caution

Before merging, carousel.md modifications must be reverted

Description

This work has been done in collaboration with @louismaximepiton, please don't remove the "Co-authored-by" from the commit message if merged

This PR tackles specifically #40493. This work is based on #39295 spirit.
The idea is to rely on custom properties so that there's never a specificity issue when nesting color modes (light mode in dark mode, and dark mode in light mode).

In order for it to be non-breaking, _variables-dark.scss uses the newly deprecated $carousel-dark-* Sass variables.

Please note that in the future, we'll be able to replace :root, [data-bs-theme="light"] { with @include color-mode(light, true) as suggested in #39295.

This part generates the following CSS:

.carousel-dark {
  --bs-carousel-indicator-active-bg: #000;
  --bs-carousel-caption-color: #000;
  --bs-carousel-control-icon-filter: invert(1) grayscale(100);
}

:root,
[data-bs-theme=light] {
  --bs-carousel-indicator-active-bg: #fff;
  --bs-carousel-caption-color: #fff;
  --bs-carousel-control-icon-filter: ;
}

[data-bs-theme=dark] {
  --bs-carousel-indicator-active-bg: #000;
  --bs-carousel-caption-color: #000;
  --bs-carousel-control-icon-filter: invert(1) grayscale(100);
}

After this PR

After this PR, we might do exactly the same thing for the problematic rendering of the accordions, close buttons, etc. and use the same technique.

@mdo and @twbs/css-review what do you think about this PR? Do you agree that it can be used, it doesn't bring regressions, and can be replicated for other issues (accordions, etc.)?

Type of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist

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

Live previews

Related issues

Closes #40493

Co-authored-by: Louis-Maxime Piton <[email protected]>
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.

I'm fine with that, to be honest I still don't know if I'd prefer to have it in the _root.scss or in each component but the mechanism is great to me regarding the color mode approach.

@julien-deramond
Copy link
Member Author

I'm fine with that, to be honest I still don't know if I'd prefer to have it in the _root.scss or in each component but the mechanism is great to me regarding the color mode approach.

For now, I prefer to have it in each component, as if one doesn't want to embed the accordion in your custom build, we wouldn't have extra useless rules.

@julien-deramond julien-deramond marked this pull request as ready for review August 7, 2024 07:32
@julien-deramond julien-deramond requested a review from a team as a code owner August 7, 2024 07:32
@julien-deramond
Copy link
Member Author

julien-deramond commented Dec 19, 2024

Reverted the carousel.md change used to review the PR. Let's merge it.

@julien-deramond julien-deramond merged commit 0cbfe13 into main Dec 19, 2024
15 checks passed
@julien-deramond julien-deramond deleted the main-jd-carousel-prev-next-dark-theme branch December 19, 2024 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When on dark theme, carousel child element set with light theme not displayed properly
2 participants