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

Add ::selection to color-mode-theme() mixin #2472

Merged
merged 4 commits into from
Jun 28, 2023
Merged

Add ::selection to color-mode-theme() mixin #2472

merged 4 commits into from
Jun 28, 2023

Conversation

simurai
Copy link
Contributor

@simurai simurai commented Jun 23, 2023

What are you trying to accomplish?

This fixes #2411 by adding ::selection to the color-mode-theme() mixin.

What approach did you choose and why?

It's similar to #2437 that needed to be reverted again. But in this attempt, the ::selection is only added to the color-mode-theme() mixin. To make the ::selection work with CSS variables and all color modes, I had to add it everywhere @content is used. It's a bit cumbersome, but not sure if there is a better approach? 🤔

A usage example of

@include color-mode-theme(light, true) {
  --color: red;
}

should output:

Before

:root,
[data-color-mode=light][data-light-theme=light],
[data-color-mode=dark][data-dark-theme=light] {
  --color: red;
}

@media (prefers-color-scheme: light) {
  [data-color-mode=auto][data-light-theme=light] {
    --color: red;
  }
}
@media (prefers-color-scheme: dark) {
  [data-color-mode=auto][data-dark-theme=light] {
    --color: red;
  }
}

After

:root,
:root::selection,
[data-color-mode=light][data-light-theme=light],
[data-color-mode=light][data-light-theme=light]::selection,
[data-color-mode=dark][data-dark-theme=light],
[data-color-mode=dark][data-dark-theme=light]::selection {
  --color: red;
}

@media (prefers-color-scheme: light) {
  [data-color-mode=auto][data-light-theme=light],
  [data-color-mode=auto][data-light-theme=light]::selection {
    --color: red;
  }
}
@media (prefers-color-scheme: dark) {
  [data-color-mode=auto][data-dark-theme=light],
  [data-color-mode=auto][data-dark-theme=light]::selection {
    --color: red;
  }
}

Downside is that we can't use this mixin with a class anymore, like

@include color-mode-theme(dark) {
  .my-class {
    --color: red;
  }
}

since that would output ... ::selection .my-class and as seen, make the selector invalid and the entire ruleset is discarded. 😩 I added a warning as a comment. At some point we hopefully can remove the &, &::selection {} part again (once the spec/implementation changes). 🙏

More infos:

What should reviewers focus on?

Note that this issue is currently only reproducible with Chrome's chrome://flags/#enable-experimental-web-platform-features enabled.

I'll try to test this first with a canary version on dotcom to avoid any more surprises.

Can these changes ship as is?

  • Yes, this PR does not depend on additional changes. 🚢

@changeset-bot
Copy link

changeset-bot bot commented Jun 23, 2023

🦋 Changeset detected

Latest commit: b90c40a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/css Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@simurai simurai changed the title Add ::selection only to the color-mode-theme() mixin Add ::selection to color-mode-theme() mixin Jun 23, 2023
@github-actions github-actions bot temporarily deployed to Storybook Preview June 23, 2023 09:50 Inactive
}
}
}

// This mixin wraps styles with light or dark mode selectors
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comment is unrelated, but maybe good to document the color-mode() mixin too.

Copy link
Member

@keithamus keithamus left a comment

Choose a reason for hiding this comment

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

This LGTM 👍

AFAICT color-mode-theme isn't used in dotcom so this should land safely.

@simurai simurai marked this pull request as ready for review June 26, 2023 05:31
@simurai simurai requested a review from a team as a code owner June 26, 2023 05:31
@simurai simurai requested a review from rezrah June 26, 2023 05:31
@simurai
Copy link
Contributor Author

simurai commented Jun 26, 2023

AFAICT color-mode-theme isn't used in dotcom so this should land safely.

Yeah, I haven't seen it being used outside of PCSS. Maybe in the next major version we could move it into color-modes/ to make it a bit more private and not part of the shared mixins.

Copy link
Contributor

@langermank langermank left a comment

Choose a reason for hiding this comment

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

Nice work! 👏

@simurai simurai merged commit 3ee117e into main Jun 28, 2023
@simurai simurai deleted the selection branch June 28, 2023 05:30
@primer-css primer-css mentioned this pull request Jun 28, 2023
@langermank langermank restored the selection branch July 17, 2023 16:44
@langermank langermank temporarily deployed to github-pages July 17, 2023 16:45 — with GitHub Pages Inactive
@langermank langermank temporarily deployed to github-pages July 17, 2023 16:48 — with GitHub Pages Inactive
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.

Highlight color is muted
3 participants