Skip to content

Nesting light mode components under dark mode#38065

Closed
mdo wants to merge 1 commit intomainfrom
carousel-nested-color-modes
Closed

Nesting light mode components under dark mode#38065
mdo wants to merge 1 commit intomainfrom
carousel-nested-color-modes

Conversation

@mdo
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

julien-deramond commented Feb 19, 2023

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
Copy Markdown
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
Copy Markdown
Member

julien-deramond commented Mar 2, 2023

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
Copy Markdown
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