Skip to content

Add a custom selector for dark mode - #243

Open
davidfischer wants to merge 2 commits into
mainfrom
davidfischer/custom-selector-dark-mode
Open

Add a custom selector for dark mode#243
davidfischer wants to merge 2 commits into
mainfrom
davidfischer/custom-selector-dark-mode

Conversation

@davidfischer

@davidfischer davidfischer commented Sep 2, 2026

Copy link
Copy Markdown
Contributor
  • Adds a new attribute data-ea-dark-selector which when set will be monitored and toggle light/dark mode based on changes to it. This is for sites that use custom themes that don't use html.dark or similar patterns.
  • Note that the adaptive-css class is NOT required if this custom attribute is set
  • Add docs for the placement
  • If the theme toggle attribute is not on body or html, then the ad will correctly load in dark/light mode based on the user's custom attribute/class but if the ad is rotated or the theme is custom toggled, then the watcher won't catch it because it's only watching for changes to html/body and only on attributes. It doesn't look at descendant nodes which should keep this from slowing anything down.

Testing

To test this out manually, you can do the following:

npm run dev

# Open the browser
open http://localhost:8080/

# Open the inspector, add an attribute `data-color-scheme='slate'` on the HTML element
# Observe that the "dark" class was added to the top ad placement
# Remove/change the `data-color-scheme` attribute and observe that the ad changes back to light mode

- Adds a new attribute `data-ea-dark-selector` which when set will be
  monitored and toggle light/dark mode based on changes to it. This is
  for sites that use custom themes that don't use `html.dark` or similar
  patterns.
- Note that the `adaptive-css` class is *NOT* required if this custom
  attribute is set
- Add docs for the placement
@ericholscher

Copy link
Copy Markdown
Member

This seems like a good approach to make something a bit more generic, though I doubt many publishers will use it, but at least we have a solution.

@davidfischer

Copy link
Copy Markdown
Contributor Author

This seems like a good approach to make something a bit more generic, though I doubt many publishers will use it, but at least we have a solution.

Ya, it would give us a solution for just about any theme.

@davidfischer
davidfischer marked this pull request as ready for review September 3, 2026 15:31
@davidfischer

Copy link
Copy Markdown
Contributor Author

I tested this out, added a few notes, and added testing instructions. I think this is close to merge-ready.

@ericholscher ericholscher left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks great -- AI review only caught this one suggestion.

Comment thread docs/index.rst

.. code:: html

<div class="raised" data-ea-publisher="..." data-ea-dark-selector="body[data-md-color-scheme='slate']"></div>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Definitely don't love the mixed quoting here, but unavoidable?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ya, I don't think it's avoidable but should be rare.

Comment thread index.js
@@ -1259,6 +1329,10 @@ export function unload_placements() {
elements.forEach((div) => {
div.innerHTML = "";
div.classList.remove("loaded");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
div.classList.remove("loaded");
div.classList.remove("loaded", "dark");

Ai suggested:

applyDarkSelector() toggles the dark class on the placement wrapper itself, but unload_placements() only removes loaded and disconnects the observer. It doesn’t remove dark, so a placement that was previously dark can keep that class across unload/reload cycles and briefly or persistently render with stale theme state.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't think we want this suggestion. When the placement is explicitly set to dark mode (say, by the user explicitly) and then the ad is rotated for example, we want to keep it dark mode.

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