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

Dark mode CSS can be imported twice on WNPs. #15772

Open
alexgibson opened this issue Dec 18, 2024 · 1 comment · May be fixed by #15857
Open

Dark mode CSS can be imported twice on WNPs. #15772

alexgibson opened this issue Dec 18, 2024 · 1 comment · May be fixed by #15857
Labels
Bug 🐛 Something's not working the way it should P3 Third level priority - Nice to have

Comments

@alexgibson
Copy link
Member

Description

Minor foot-gun here:

See: https://github.com/mozilla/bedrock/blob/main/media/css/firefox/whatsnew/whatsnew-135beta.scss#L8-L9

@import 'includes/dark-mode';
@import 'includes/mofo-donate-cta';

Note that _mofo_donate-cta.scss also imports dark-mode directly: https://github.com/mozilla/bedrock/blob/main/media/css/firefox/whatsnew/includes/_mofo-donate-cta.scss#L6

We should probably remove the dark_mode import from the donate scss, and make both explicit.

@alexgibson alexgibson added Bug 🐛 Something's not working the way it should P3 Third level priority - Nice to have labels Dec 18, 2024
@Ayushsunny Ayushsunny linked a pull request Jan 14, 2025 that will close this issue
1 task
@janbrasna
Copy link
Contributor

TBH the real issue actually seems to be the other way around — when dark-mode not specified and included, but silently smuggled in via mofo-donate-cta, there are dark-mode-enabled WNP pages now that only work in dark mode due to this issue. Once the issue above is resolved, all the WNP 132-135b without explicit dark-mode partial that now adapt to color schemes will be light only. (Bug or feature? A change/regression nonetheless. Any update needs to be made along with any fixes needed for the cases below, as demonstrated in #15857 (review) …)

Quickly glancing the rules:

Thinking aloud:

  • _mofo-donate-cta.scss doesn't actually include any special CTA styles, only dark mode tweaks; so loading it on pages that are not dark mode is superfluous, only now it serves as an equivalent to importing _dark-mode.scss when not otherwise loaded;D
  • it can be either merged to _dark-mode.scss to not have to think about it again, with the caveat of serving the extra lines even in pages not including the CTA… (most currently are)
  • … or it can be renamed to something like _dark-mode-mofo-donate-cta.scss to be obviously apparent what purpose it serves, and that it only needs to accompany dark-mode import, but the actual dark-mode import has to be made explicitly too.
  • In any case some of the pages need either removing this CTA import, adding the dark-more import, or a decision where the theming was intended to be a feature and should be coded to stay, and where it was merely a coincidence/bug and it should be coded out explicitly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 🐛 Something's not working the way it should P3 Third level priority - Nice to have
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants