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.

Sign up for GitHub

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

Jump to bottom

Nesting light mode components under dark mode #38065

Closed
wants to merge 1 commit into from
Closed

Nesting light mode components under dark mode #38065

wants to merge 1 commit into from

Conversation

mdo
Copy link
Member

@mdo mdo commented Feb 15, 2023

Something we've overlooked is how to properly nest light mode inside a parent dark mode. I ran into this while updating the examples to better support color modes. Basically, if you have the root set to dark mode and you want something like our carousel to appear in light mode, doing the following won't work:

<html data-bs-theme="dark">
  <div class="carousel" data-bs-theme="light">...</div>
</html>

This is because we have no selector for [data-bs-theme="dark"] .carousel { ... } in our codebase. We only have .carousel { ... }. As such, this PR suggests a potential solution we could use across the carousel component and other components: use a :not() selector in the selector.

@if $enable-dark-mode {
  @include color-mode(dark) {
    .carousel:not([data-bs-theme="light"]) {
      @include carousel-dark();
    }
  }
}

The same changes might need to be applied to accordions, dropdowns, close button, navbars, form checks, and form selects (all places we call the color mode mixin right now).

Thoughts?

@mdo mdo requested a review from a team as a code owner February 15, 2023 19:54
@voltaek
Copy link
Contributor

voltaek commented Feb 16, 2023

This solution doesn't address if an intermediate parent element has [data-bs-theme="light"] on it, though, if I'm understanding this correctly.

It seems like .carousel might need a more in-depth refactor to keep supporting .carousel-dark in v5 while also supporting the new colors modes. Would switching the component over to using more CSS variables help? Or maybe there's not a great way to do it while maintaining v5 compatibility and it needs to wait for v6 due to having to keep supporting both methods of "coloring" in v5.

@julien-deramond
Copy link
Member

My first feedback is the same as @voltaek regarding intermediate data-bs-themes. The following won't work:

<html data-bs-theme="dark">
  <div data-bs-theme="light">
     <div class="carousel">...</div>
  </div>
</html>

Going to try to think about another approach but I don't think about something else right now 🤔

@mdo
Copy link
Member Author

mdo commented Mar 2, 2023

We might just need to remove the nesting for v5 and revisit in v6. I know there was an issue or PR that tried to address this in the past from a contributor—it basically quadrupled the number of selectors. Can't find the link now though.

@julien-deramond
Copy link
Member

We might just need to remove the nesting for v5 and revisit in v6.

If we don't find a proper solution in v5, I'm down to remove the nesting in v5 and try to tackle it entirely in v6. IMO I'd say that this limitation would be better identified by the users in terms of communication if we do it like this.

@mdo
Copy link
Member Author

mdo commented Mar 8, 2023

Removing the docs section in #38192.

@mdo mdo closed this Mar 8, 2023
@mdo mdo deleted the carousel-nested-color-modes branch March 8, 2023 05:41
@julien-deramond julien-deramond mentioned this pull request Mar 8, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
css v5
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mdo @voltaek @julien-deramond