Uh oh!
There was an error while loading. Please reload this page.
Let CodeBlock resolve its theme from a scope - #120
Merged
Conversation
CodeBlock is the only component that branches on the theme in JavaScript — it picks a syntax palette — and it reads the page-wide theme state to do it. That value is a single global resolved by walking the document, so it is wrong in two situations: - More than one theme on screen. The first root to mount decides, so a dark subtree renders the light palette. Concretely: the light palette is stock Prism, which sets `text-shadow: 0 1px white`, and a white halo on a dark surface renders every glyph doubled and close to unreadable. - Shadow DOM. The fallback resolves the theme with `document.querySelector`, which cannot see into a shadow tree. Resolve it as prop, then context, then the existing page-wide state: <CodeBlock theme="dark" … /> // explicit <ThemeProvider theme="dark">…</ThemeProvider> // via ThemeProvider <ThemeScopeProvider theme="dark">…</…> // theming a subtree yourself Backwards compatible: with no prop and no scope, behaviour is unchanged, so existing consumers are unaffected. `ThemeProvider`'s new `theme` prop is optional and only supplies the scope — it does not change what it renders. Longer term this component should not read the theme in JavaScript at all. Every other component themes through CSS custom properties on the nearest `[data-equality-theme]` ancestor, which is already correct per-instance, inside shadow roots, and on first paint. Expressing the two syntax palettes as tokens and passing `var(--…)` references to the highlighter would remove this branch, the flash before the theme resolves, and the side effect where reading the page-wide state writes `data-equality-theme` onto `document.documentElement`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Mirrors the "Portalled surfaces" section added for the portal container: the two failure cases are the same, so they read as a pair. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Shrinks99
requested changes
Jul 31, 2026
Shrinks99
left a comment
Member
There was a problem hiding this comment.
No other elements other than themeprovider have the option to set dark and light on them. I really don't want to set this precedent, all elements should inherit the dark and light mode from the theme provider's setting.
CodeBlock took a `theme` prop and ThemeScopeProvider was exported, so a theme could be declared in two places besides ThemeProvider. Drop both: the scope context is internal now, set only by `<ThemeProvider theme>`, and CodeBlock reads it through context with the page-wide theme as its fallback. A nested provider without a `theme` still inherits the one it sits in rather than clearing it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…-scope # Conflicts: # packages/ui/src/theme/theme-provider.tsx
Shrinks99
requested changes
Aug 17, 2026
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
A host that embeds Equality into its own layout cannot always afford a second ThemeProvider: it brings the scoped theme CSS and an unstyled wrapper element with it, and a host's layout may not survive either. What those hosts want is only the theme declaration, so keep that available on its own. ThemeProvider stays the way an app sets a theme, and CodeBlock still has no theme prop of its own — it reads the scope, so a subtree cannot disagree with itself about which theme it is on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review feedback: don't document things that don't exist or that are standard behaviour, and don't imply ThemeProvider wraps each component rather than the app. Specifically, drop the sentences describing the absence of a `theme` prop on components, and the example wrapping a lone CodeBlock in a ThemeProvider. Generally, the same cuts apply to the ThemeProvider and theme scope JSDoc, and the ThemeScopeProvider example now wraps a subtree rather than a single code block. The code block page keeps only what is particular to it — that it resolves the theme in JavaScript, unlike everything else — and links to the usage page for how to set one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
makew0rld
commented
Aug 17, 2026
ContributorAuthor
I've added back |
makew0rld
commented
Aug 17, 2026
ContributorAuthor
Will be updating this PR after call. Best case: API surface doesn't change |
CodeBlock was the only component in the library that branched on the theme in JavaScript. It did so because its palette arrived as a JS object that the highlighter applied as inline styles on every token span, which meant the component had to know the theme in order to pick one. That knowledge came from useTheme, which resolves a single page-wide value via document.querySelector. It returns the first match in document order, so a second theme root on the page gets the wrong palette, and it cannot cross a shadow boundary, so a code block inside a shadow root gets the wrong palette too. ThemeScopeProvider addressed that by threading the answer through context, but it made every embedder declare in JavaScript a theme the DOM already declares in CSS, and left the inline styles in place. Expressing the palette as CSS removes the question instead: the dark: variant matches against the nearest theme root, so multiple themes, shadow roots and nested themes are all correct by construction, and a theme flip needs no re-render. Values are ported from the highlighter's prism and a11y-dark themes, grouped into the twelve roles that share both a light and a dark value. Three of the palette's properties were quietly winning arguments they should have lost, and are now fixed by their absence: - font-family: Consolas beat .content's var(--font-mono), because that is on the parent and only inherited. Code blocks now use the mono token. - padding: 1em beat .pre's padding-block, which carried no !important, so the intended py-3 had no effect. - text-shadow: 0 1px white had nothing competing, and rendered as doubled glyphs whenever the JS lookup picked the light palette on a dark surface. .pre's four !important declarations existed solely to out-rank those inline styles and are gone with them. Reverts ThemeScopeProvider, useThemeScope and ThemeProvider's theme prop, none of which have a remaining purpose. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
makew0rld
commented
Aug 17, 2026
ContributorAuthor
Ok, things are way cleaner now. The tradeoff is it's harder to change themes, since the CSS values are inlined, but this way we get a cascade and scoping automatically. The public API surface is unchanged. If theme switching is not a concern, this PR should be good. The other option would be to import and build this CSS file at build time instead. |
yes in a comment, apparently this is a thing?
This is subject to change
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CodeBlock is the only component that branches on the theme in JavaScript and it reads the page-wide theme state to do it. That value is a single global resolved by walking the document, so it is wrong in two situations:
text-shadow: 0 1px white, and a white halo on a dark surface renders every glyph doubled and close to unreadable.document.querySelector, which cannot see into a shadow tree.Resolve it as prop, then context, then the existing page-wide state:
Backwards compatible: with no prop and no scope, behaviour is unchanged, so existing consumers are unaffected.
ThemeProvider's newthemeprop is optional and only supplies the scope — it does not change what it renders.Longer term this component should not read the theme in JavaScript at all. Every other component themes through CSS custom properties on the nearest
[data-equality-theme]ancestor, which is already correct per-instance, inside shadow roots, and on first paint. Expressing the two syntax palettes as tokens and passingvar(--…)references to the highlighter would remove this branch, the flash before the theme resolves, and the side effect where reading the page-wide state writesdata-equality-themeontodocument.documentElement.PR text written by Claude Code, reviewed by me