Skip to content

Force color-mode mixins to prepend selectors instead of append #39194

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
Open
chez14 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Force color-mode mixins to prepend selectors instead of append #39194

chez14 wants to merge 1 commit into from

Conversation

@chez14
Copy link

@chez14 chez14 commented Sep 16, 2023 •
edited
Loading

Description

color-mode mixins on $color-mode-type=data appends [data-bs-theme="#{$mode}"] instead of prepend the selectors from the parent. This PR will update the behavior of color-mode mixins on $color-mode-type=data so that we can use this mixins deep inside other selector/rule blocks.

Motivation & Context

Before this PR, using the color-mode mixins inside a rule block will just append [data-bs-theme="#{$mode}"] selector to the parent selector.

SCSS:

.parent {
  @include color-mode(dark, true) {
    .element {
      color: var(--bs-primary-text-emphasis);
      background-color: var(--bs-primary-bg-subtle);
    }
  }
  @include color-mode(dark, true) {
    --custom-color: #{mix($indigo, $blue, 50%)};
  }
}

Output:

.parent [data-bs-theme=dark] .element {
  color: var(--bs-primary-text-emphasis);
  background-color: var(--bs-primary-bg-subtle);
}
.parent [data-bs-theme=dark] {
  --custom-color: #3a3ff8;
}

This is problematic, because if [data-bs-theme=dark] are set on <html>tag, the selector will not match. So this PR update the said SCSS code to following CSS:

[data-bs-theme=dark] .parent .element {
  color: var(--bs-primary-text-emphasis);
  background-color: var(--bs-primary-bg-subtle);
}
[data-bs-theme=dark] .parent {
  --custom-color: #3a3ff8;
}

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

Related issues

@chez14 chez14 marked this pull request as ready for review September 16, 2023 14:30
@chez14 chez14 requested a review from a team as a code owner September 16, 2023 14:30
@julien-deramond julien-deramond mentioned this pull request Sep 19, 2023
Merged
@julien-deramond
Copy link
Member

julien-deramond commented Aug 6, 2024

Thanks for your PR @chez14, and sorry for the delay.
I'm not sure to catch the exact use case that wouldn't work with the actual behavior.
Based on your example, if we do the following

@include color-mode(dark) {
  .parent {
    --custom-color: #{mix($indigo, $blue, 50%)};

    .element {
      color: var(--bs-primary-text-emphasis);
      background-color: var(--bs-primary-bg-subtle);
    }
  }
}

we'd get the same result as you:

[data-bs-theme=dark] .parent {
 --custom-color: #3a3ff8;
}
[data-bs-theme=dark] .parent .element {
 color: var(--bs-primary-text-emphasis);
 background-color: var(--bs-primary-bg-subtle);
}

Could you please try to present a blocking use case on your side that we could analyze?
Otherwise, we'll close this PR.

@chez14
Copy link
Author

chez14 commented Aug 14, 2024 •
edited
Loading

Hi @julien-deramond, sorry for the late reply.

This PR tries to mimic the @media query behavior in nested or more complex selector. While your suggested solution works, I don't feel like this will be scalable with more complex selector.

In my case, I have this notice in the footer (.footer-final-notice class) that I want it to be in darker gray on light mode (this particular project were made in dark-mode only).

.footerCopyright {
    color: $wandering-white-700;
    background-color: $gloomy-gray-700;
    padding: 1rem 0rem;

    --#{$prefix}link-color-rgb: #{to-rgb($wandering-white-700)};
    --#{$prefix}link-hover-color-rgb: #{to-rgb($wandering-white-600)};

    p {
        margin: 0rem;
    }

    :global {
        .footer-final-notice {
            margin-top: 0.5rem;

            @include media-breakpoint-up(md) {
                margin-top: 0rem;
                text-align: right;
            }
        }
    }
}

With your solution, if I want to change the color in light mode, I have to write the class names twice for it to be rendered properly in both $color-mode-types:

.footerCopyright {
   /* ... */
    :global {
        .footer-final-notice {
            /* ... */
        }
    }
}

@include color-mode(light) {
    .footerCopyright :global(.footer-final-notice) {
        color: $color-gray-600;
    }
}

If this class have more nested classes (such as BEM), the overrides will be far in the bottom of the parent class, and will usually lead to maintainability issues.

With this PR, I can now implement it just like media-breakpoint-up mixins:

.footerCopyright {
   /* ... */
    :global {
        .footer-final-notice {
            margin-top: 0.5rem;

            @include color-mode(light) {
                color: $color-gray-600;
            }

            @include media-breakpoint-up(md) {
                margin-top: 0rem;
                text-align: right;
            }
        }
    }
}

I hope this can explain what I'm trying to solve 😅. Please let me know if you have any question, or I'm doing it wrong.

Thank you!

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

color mode css v5

Projects

v5.4.0
Status: Needs review

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@chez14 @julien-deramond @louismaximepiton