Skip to content

Remove duplicate dark mode styles#2612

Open
nqst wants to merge 13 commits intobasecamp:mainfrom
nqst:dry-dark-mode-styles
Open

Remove duplicate dark mode styles#2612
nqst wants to merge 13 commits intobasecamp:mainfrom
nqst:dry-dark-mode-styles

Conversation

@nqst
Copy link
Contributor

@nqst nqst commented Feb 25, 2026

This PR removes duplicate dark mode styles. They are currently defined twice, which is error-prone and hard to maintain.

Here, we rely on [data-theme] only. JavaScript resolves the theme and sets data-theme to light or dark in all cases, including auto mode.

Changes:

  • Resolves data-theme to light or dark at load time
  • Cleans up CSS files significantly by removing @media (prefers-color-scheme: dark) blocks
  • Removes light-dark() from syntax.css for consistency
  • Adds a missing & in syntax.css to fix incorrect variable scoping in dark mode
  • Mounts the theme controller in the public layout so the auto theme responds to OS appearance changes

Copilot AI review requested due to automatic review settings February 26, 2026 08:17
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR consolidates dark mode styling by relying exclusively on html[data-theme] (always set to light or dark), removing duplicated prefers-color-scheme CSS fallbacks and aligning syntax highlighting styles with the same approach.

Changes:

  • Resolve data-theme to light/dark at load time (including “auto”) and update the theme controller to react to OS appearance changes.
  • Remove @media (prefers-color-scheme: dark) fallback blocks across stylesheets in favor of html[data-theme="dark"] selectors.
  • Simplify and correct syntax highlighting dark-mode scoping and replace light-dark() usage with explicit theme selectors.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
app/views/layouts/public.html.erb Mounts the theme Stimulus controller on public pages so auto mode reacts to OS changes.
app/views/layouts/_theme_preference.html.erb Sets documentElement.dataset.theme to a resolved light/dark value at parse-time to avoid duplicate CSS fallbacks.
app/javascript/controllers/theme_controller.js Resolves “auto” to light/dark and listens for system theme changes to re-apply.
app/assets/stylesheets/trays.css Removes prefers-color-scheme fallback in favor of html[data-theme="dark"].
app/assets/stylesheets/syntax.css Fixes dark-mode variable scoping with & nesting and replaces light-dark() with explicit theme rules.
app/assets/stylesheets/reactions.css Removes prefers-color-scheme fallback in favor of html[data-theme="dark"].
app/assets/stylesheets/notifications.css Replaces media-query-based dark styling with html[data-theme="dark"] selector.
app/assets/stylesheets/markdown.css Removes prefers-color-scheme fallback in favor of html[data-theme="dark"].
app/assets/stylesheets/inputs.css Removes prefers-color-scheme fallback in favor of html[data-theme="dark"].
app/assets/stylesheets/circled-text.css Removes prefers-color-scheme fallback in favor of html[data-theme="dark"].
app/assets/stylesheets/cards.css Removes prefers-color-scheme fallback in favor of html[data-theme="dark"].
app/assets/stylesheets/card-columns.css Removes prefers-color-scheme fallback in favor of html[data-theme="dark"].
app/assets/stylesheets/buttons.css Removes prefers-color-scheme fallback in favor of html[data-theme="dark"].
app/assets/stylesheets/base.css Removes prefers-color-scheme fallback in favor of html[data-theme="dark"].
app/assets/stylesheets/bar.css Removes prefers-color-scheme fallback in favor of html[data-theme="dark"].
app/assets/stylesheets/_global.css Removes the global prefers-color-scheme: dark fallback block now that data-theme is expected to always be set.
Comments suppressed due to low confidence (1)

app/assets/stylesheets/_global.css:373

  • Removing the prefers-color-scheme fallback means users without JavaScript (or if the inline theme script fails to run) will no longer get dark mode based on system preference, since only html[data-theme="dark"] is now supported. If no-JS support matters, consider keeping a minimal prefers-color-scheme fallback (or setting data-theme server-side) so the site still respects OS appearance without JS.
    0 0.4em 0.8em -1.2em oklch(var(--lch-black) / 0.8),
    0 0.8em 1.2em -1.6em oklch(var(--lch-black) / 0.9),
    0 1.2em 1.6em -2em oklch(var(--lch-black) / 1);
}


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 52 to 55
const applyTheme = () => {
if (theme === "auto") {
document.documentElement.removeAttribute("data-theme")
} else {
document.documentElement.setAttribute("data-theme", theme)
}

document.documentElement.dataset.theme = resolved
this.#updateButtons()
}
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

Theme is now resolved to light/dark independent of OS preference, but the app’s tags (in layouts/shared/_head.html.erb) are still driven by prefers-color-scheme media queries. This can leave the browser UI color out of sync when a user forces light on a dark OS (or vice versa). Consider updating theme_controller to also update/replace the theme-color meta tag based on the resolved theme whenever applying the theme.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@nqst nqst Feb 26, 2026

Choose a reason for hiding this comment

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

Very nice find! This issue wasn't introduced by this branch — in the current main, the theme-color meta tags rely only on the OS preferences and won't be updated if you set a different theme in Profile settings. But of course it's nice to fix it.

Addressed this in b09bff5

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Simplified it in 68965d6

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After some more testing, I decided to revert the theme-color changes. I noticed a brief window frame flash in the Chrome web app, and it looks buggy. I was able to fix this, but I wasn't happy with the code and the overall complexity. The goal of this PR is to remove the duplicate CSS in the first place, and I'm not even sure if the team would want to remove the native (prefers-color-scheme: dark) styles. I don't think it's a good idea to overcomplicate it.

The theme-color issue is valid, but I think it makes more sense to address it separately in its own PR.

Copilot AI review requested due to automatic review settings February 26, 2026 16:54
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants