feat(theme): scoped theming with ThemeScope component#768
Open
paanSinghCoder wants to merge 11 commits into
Open
feat(theme): scoped theming with ThemeScope component#768paanSinghCoder wants to merge 11 commits into
paanSinghCoder wants to merge 11 commits into
Conversation
Any element with `data-theme` (or wrapped in `<ThemeScope>`) now creates an isolated theme region. Descendants resolve all `--rs-color-*` tokens, `color-scheme`, and the smooth theme-switch transition from the nearest scoped ancestor. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces scoped theming: Theme now detects an existing ThemeContext and, when nested, renders a scoped wrapper that emits data-theme and optional data-* token attributes (style, accent/gray). It adds persistent scoped behavior via storageKey, expands types to centralized token lists and optional token fields, re-exports Theme, adds tests for stateless and persistent scopes (including cross-tab sync), provides an interactive example page, updates docs, and scopes CSS variables to data-theme attributes.
Sequence Diagram(s)sequenceDiagram
participant Client
participant RootTheme as Theme(root)
participant NestedTheme as Theme(nested)
participant Wrapper as ScopedWrapper
participant Storage as localStorage
Client->>RootTheme: mount root ThemeProvider
Client->>NestedTheme: mount nested Theme(storageKey?/overrides)
NestedTheme->>RootTheme: detect parent ThemeContext
NestedTheme->>Wrapper: render wrapper with data-theme / data-* attrs
Wrapper->>Storage: read (on mount) / write (on setTheme) using storageKey
Storage-->>Wrapper: storage changes (storage event) -> update wrapper attrs
Possibly related issues
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/www/src/app/examples/scoped-theme/page.tsx`:
- Around line 17-23: The panelStyle object uses a spacing token for borderRadius
(borderRadius: 'var(--rs-space-3)') which teaches the wrong token family; update
the borderRadius value to use the appropriate radius token (e.g., replace
var(--rs-space-3) with the project's radius token such as var(--rs-radius-3) or
the correct --rs-radius-<n>), keeping the rest of panelStyle unchanged so the
example demonstrates the proper radius token usage.
In `@packages/raystack/styles/primitives/appearance.css`:
- Around line 13-20: The CSS unconditionally applies color-scheme when
[data-theme] is present, bypassing ThemeProvider's enableColorScheme flag;
update the rules to only apply when an explicit opt-in marker is present (e.g.,
[data-enable-color-scheme="true"] or a class like .enable-color-scheme) so
ThemeProvider's enableColorScheme prop in ThemeProvider
(packages/raystack/components/theme-provider/theme.tsx) can control whether
native color-scheme is set; ensure the marker is added/removed by ThemeProvider
where enableColorScheme is read so disabling the prop actually prevents the CSS
color-scheme rules from applying.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 68a8df63-d9e9-4b91-beba-bc0d1b81312d
📒 Files selected for processing (13)
apps/www/src/app/examples/scoped-theme/page.tsxapps/www/src/content/docs/theme/overview/index.mdxapps/www/src/content/docs/theme/overview/props.tspackages/raystack/components/theme-provider/__tests__/theme-scope.test.tsxpackages/raystack/components/theme-provider/index.tsxpackages/raystack/components/theme-provider/theme-scope.tsxpackages/raystack/components/theme-provider/theme.tsxpackages/raystack/components/theme-provider/types.tspackages/raystack/index.tsxpackages/raystack/styles/colors.csspackages/raystack/styles/primitives/accent.csspackages/raystack/styles/primitives/appearance.csspackages/raystack/styles/primitives/gray.css
Comment on lines
+13
to
+20
| /* Native UI (form controls, scrollbars, text selection) follows the scoped theme */ | ||
| [data-theme="light"] { | ||
| color-scheme: light; | ||
| } | ||
|
|
||
| [data-theme="dark"] { | ||
| color-scheme: dark; | ||
| } |
Contributor
There was a problem hiding this comment.
enableColorScheme={false} is bypassed by unconditional scoped color-scheme.
ThemeProvider still models enableColorScheme as a conditional behavior (packages/raystack/components/theme-provider/theme.tsx, Line 40 and Lines 90-99 / 258-271), but this CSS now forces native color-scheme whenever data-theme is present. That makes the prop effectively non-functional.
Please gate these rules behind an explicit opt-in attribute/class so disabling enableColorScheme is still respected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/raystack/styles/primitives/appearance.css` around lines 13 - 20, The
CSS unconditionally applies color-scheme when [data-theme] is present, bypassing
ThemeProvider's enableColorScheme flag; update the rules to only apply when an
explicit opt-in marker is present (e.g., [data-enable-color-scheme="true"] or a
class like .enable-color-scheme) so ThemeProvider's enableColorScheme prop in
ThemeProvider (packages/raystack/components/theme-provider/theme.tsx) can
control whether native color-scheme is set; ensure the marker is added/removed
by ThemeProvider where enableColorScheme is read so disabling the prop actually
prevents the CSS color-scheme rules from applying.
Contributor
Author
|
@rohilsurana Closing this PR for now. We will pick this later when we have defined usecase and better scoping. |
ThemeProvider auto-detects scope vs root via context: nested usage renders a wrapper `<div>` with `data-*` overrides instead of a no-op fragment. Drops the ThemeScope component (and its render-prop/Base UI dep) so consumers have one entry point. useTheme() inside a scope still returns the root provider's state — scoped mode is stateless by design. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/raystack/components/theme-provider/__tests__/theme-scope.test.tsx (1)
6-27: ⚡ Quick winIsolate and restore global
windowmocks to avoid cross-suite leakage.Lines 6–27 define
localStorageandmatchMediaat module scope without restoration, allowing mocks to leak across test suites and create order-dependent failures. Move setup intobeforeAlland restore original property descriptors inafterAll.Proposed refactor
-import { describe, expect, it, vi } from 'vitest'; +import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest'; import { ThemeProvider } from '../theme'; -// jsdom doesn't ship these; the root ThemeProvider needs them on mount. -Object.defineProperty(window, 'localStorage', { - value: { - getItem: vi.fn(), - setItem: vi.fn(), - removeItem: vi.fn(), - clear: vi.fn() - } -}); - -Object.defineProperty(window, 'matchMedia', { - writable: true, - value: vi.fn().mockImplementation(query => ({ - matches: false, - media: query, - onchange: null, - addListener: vi.fn(), - removeListener: vi.fn(), - addEventListener: vi.fn(), - removeEventListener: vi.fn(), - dispatchEvent: vi.fn() - })) -}); +const originalLocalStorage = Object.getOwnPropertyDescriptor(window, 'localStorage'); +const originalMatchMedia = Object.getOwnPropertyDescriptor(window, 'matchMedia'); + +beforeAll(() => { + Object.defineProperty(window, 'localStorage', { + configurable: true, + value: { + getItem: vi.fn(), + setItem: vi.fn(), + removeItem: vi.fn(), + clear: vi.fn() + } + }); + + Object.defineProperty(window, 'matchMedia', { + configurable: true, + writable: true, + value: vi.fn().mockImplementation(query => ({ + matches: false, + media: query, + onchange: null, + addListener: vi.fn(), + removeListener: vi.fn(), + addEventListener: vi.fn(), + removeEventListener: vi.fn(), + dispatchEvent: vi.fn() + })) + }); +}); + +afterAll(() => { + if (originalLocalStorage) { + Object.defineProperty(window, 'localStorage', originalLocalStorage); + } + if (originalMatchMedia) { + Object.defineProperty(window, 'matchMedia', originalMatchMedia); + } +});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/raystack/components/theme-provider/__tests__/theme-scope.test.tsx` around lines 6 - 27, Move the global window mocks for localStorage and matchMedia out of module scope and into a test lifecycle so they are restored after the suite: capture the original descriptors for window.localStorage and window.matchMedia, set the mocked implementations inside beforeAll (using the current mock implementations from the diff), and restore the originals in afterAll; reference the mocked symbols window.localStorage and window.matchMedia and the lifecycle hooks beforeAll/afterAll to locate where to apply the changes so other suites are not affected.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/raystack/components/theme-provider/__tests__/theme-scope.test.tsx`:
- Around line 6-27: Move the global window mocks for localStorage and matchMedia
out of module scope and into a test lifecycle so they are restored after the
suite: capture the original descriptors for window.localStorage and
window.matchMedia, set the mocked implementations inside beforeAll (using the
current mock implementations from the diff), and restore the originals in
afterAll; reference the mocked symbols window.localStorage and window.matchMedia
and the lifecycle hooks beforeAll/afterAll to locate where to apply the changes
so other suites are not affected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 38904419-ed26-4f20-9c55-f34a82462c53
📒 Files selected for processing (4)
apps/www/src/app/examples/scoped-theme/page.tsxapps/www/src/content/docs/theme/overview/index.mdxpackages/raystack/components/theme-provider/__tests__/theme-scope.test.tsxpackages/raystack/components/theme-provider/theme.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/www/src/content/docs/theme/overview/index.mdx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/www/src/app/examples/scoped-theme/page.tsx
The /examples/theme-overrides page covers the same scope-mode demo plus per-token override switches; no need to maintain two scoped-theming example routes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/raystack/components/theme-provider/theme.tsx`:
- Around line 24-33: Theme currently forces nested providers to render the
Scoped wrapper (which injects a div). Change Theme to avoid inserting a wrapper
unless the caller actually requests scoped overrides: in the Theme function,
when useContext(ThemeContext) is truthy, check for an explicit
"overrides"/"tokens"/"scope" prop (or whatever prop you use to indicate local
token overrides) and only return <Scoped {...props}/> when such overrides are
present; otherwise return props.children (or render props.children directly) so
no extra div is injected. Apply the same conditional logic to the other
nested-provider branch mentioned (lines 43-58) to ensure nested providers do not
implicitly create wrapper markup when no scoped overrides are requested.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 776f8409-7354-4c2f-b8bf-1f3dd7b29130
📒 Files selected for processing (7)
apps/www/src/app/examples/theme-overrides/page.tsxpackages/raystack/components/theme-provider/__tests__/theme-provider.test.tsxpackages/raystack/components/theme-provider/__tests__/theme-scope.test.tsxpackages/raystack/components/theme-provider/index.tsxpackages/raystack/components/theme-provider/theme.tsxpackages/raystack/index.tsxpackages/raystack/styles/effects.css
✅ Files skipped from review due to trivial changes (1)
- packages/raystack/styles/effects.css
Nested `<Theme storageKey="...">` now persists the scope's theme to localStorage and syncs across tabs. `useTheme()` returns layered state inside any nested provider — scope-relevant fields reflect overrides; for persistent scopes, `theme` + `setTheme` point at the scope's own state and storage. - `UseThemeProps`: widened `setTheme` to `(theme: string | undefined) => void`; `undefined` clears a persistent scope's storage and re-inherits (no-op at root). Added `style`/`accentColor`/`grayColor` fields (already present in the value at runtime; now declared). - `StatelessScope` and `PersistedScope` both layer scope overrides onto the parent `ThemeContext` value, so descendants reading `useTheme()` see the effective state for their subtree. - New tests cover the persistent-scope contract: localStorage read on mount, `defaultTheme` fallback, `forcedTheme` wins-but-not-persisted, cross-tab sync via `storage` event, clear-via-undefined, and nested scopes managing their own keys. - Docs: new "Persistent scope" section in theme/overview describing the priority order and FOUC caveat. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/www/src/app/examples/theme-overrides/page.tsx`:
- Around line 66-79: PersistentControls: the toggle's onClick and rendered icon
use theme directly which fails when theme is undefined (inherited/default);
change logic to derive an explicit currentTheme = theme ?? defaultLabel and use
that for both onClick and icon rendering (i.e., call setTheme(currentTheme ===
'dark' ? 'light' : 'dark') and render Sun/Moon based on currentTheme) so the
button follows the displayed scope theme; update references in
PersistentControls and keep useScopeTheme, theme, defaultLabel, setTheme names
intact.
In `@apps/www/src/content/docs/theme/overview/index.mdx`:
- Around line 156-169: Update the example snippets so they actually render a
nested ThemeProvider and show scope-local behavior of useTheme: wrap an outer
<ThemeProvider> around the shown inner <ThemeProvider> + <Card> example and
add/clarify a scoped useTheme() call inside the inner provider example to
demonstrate that useTheme() in a nested, stateful provider returns its own
theme/setTheme while in stateless nested scopes it does not; reference the
existing ThemeProvider, useTheme, and Card identifiers when editing the examples
and ensure the explanatory text mentions "stateless nested scopes" vs "stateful
nested providers" to qualify when inner useTheme() is scoped.
In `@packages/raystack/components/theme-provider/__tests__/theme-scope.test.tsx`:
- Around line 6-13: The test globals are defined with Object.defineProperty for
window.localStorage and window.matchMedia but they lack configurable:true (and
localStorage also lacks writable:true), causing "Cannot redefine property" when
other suites redefine them; update the Object.defineProperty call that sets
window.localStorage (the block creating getItem/setItem/removeItem/clear mocks)
to include configurable: true and writable: true, and update the
Object.defineProperty call that sets window.matchMedia to include configurable:
true so both globals can be redefined by other tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dfef4d48-4fca-4642-85b7-e56177649c6a
📒 Files selected for processing (6)
apps/www/src/app/examples/theme-overrides/page.tsxapps/www/src/content/docs/theme/overview/index.mdxapps/www/src/content/docs/theme/overview/props.tspackages/raystack/components/theme-provider/__tests__/theme-scope.test.tsxpackages/raystack/components/theme-provider/theme.tsxpackages/raystack/components/theme-provider/types.ts
Comment on lines
+66
to
+79
| function PersistentControls({ defaultLabel }: { defaultLabel: string }) { | ||
| const { theme, setTheme } = useScopeTheme(); | ||
| const current = theme ?? defaultLabel; | ||
| return ( | ||
| <Flex gap={3} align='center'> | ||
| <Text size={3} variant='secondary'> | ||
| <code>theme="{current}"</code> | ||
| </Text> | ||
| <IconButton | ||
| aria-label='Toggle scope theme' | ||
| size={3} | ||
| onClick={() => setTheme(theme === 'dark' ? 'light' : 'dark')} | ||
| > | ||
| {theme === 'dark' ? <Sun /> : <Moon />} |
Contributor
There was a problem hiding this comment.
Toggle/icon logic should follow the displayed scope theme.
When theme is undefined (reachable after Line 84), Line 77 always sets 'dark' and Line 79 always shows the moon icon. That breaks expected toggle behavior for inherited/default states.
Proposed fix
function PersistentControls({ defaultLabel }: { defaultLabel: string }) {
const { theme, setTheme } = useScopeTheme();
const current = theme ?? defaultLabel;
+ const isDark = current === 'dark';
return (
<Flex gap={3} align='center'>
<Text size={3} variant='secondary'>
<code>theme="{current}"</code>
</Text>
<IconButton
aria-label='Toggle scope theme'
size={3}
- onClick={() => setTheme(theme === 'dark' ? 'light' : 'dark')}
+ onClick={() => setTheme(isDark ? 'light' : 'dark')}
>
- {theme === 'dark' ? <Sun /> : <Moon />}
+ {isDark ? <Sun /> : <Moon />}
</IconButton>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function PersistentControls({ defaultLabel }: { defaultLabel: string }) { | |
| const { theme, setTheme } = useScopeTheme(); | |
| const current = theme ?? defaultLabel; | |
| return ( | |
| <Flex gap={3} align='center'> | |
| <Text size={3} variant='secondary'> | |
| <code>theme="{current}"</code> | |
| </Text> | |
| <IconButton | |
| aria-label='Toggle scope theme' | |
| size={3} | |
| onClick={() => setTheme(theme === 'dark' ? 'light' : 'dark')} | |
| > | |
| {theme === 'dark' ? <Sun /> : <Moon />} | |
| function PersistentControls({ defaultLabel }: { defaultLabel: string }) { | |
| const { theme, setTheme } = useScopeTheme(); | |
| const current = theme ?? defaultLabel; | |
| const isDark = current === 'dark'; | |
| return ( | |
| <Flex gap={3} align='center'> | |
| <Text size={3} variant='secondary'> | |
| <code>theme="{current}"</code> | |
| </Text> | |
| <IconButton | |
| aria-label='Toggle scope theme' | |
| size={3} | |
| onClick={() => setTheme(isDark ? 'light' : 'dark')} | |
| > | |
| {isDark ? <Sun /> : <Moon />} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/www/src/app/examples/theme-overrides/page.tsx` around lines 66 - 79,
PersistentControls: the toggle's onClick and rendered icon use theme directly
which fails when theme is undefined (inherited/default); change logic to derive
an explicit currentTheme = theme ?? defaultLabel and use that for both onClick
and icon rendering (i.e., call setTheme(currentTheme === 'dark' ? 'light' :
'dark') and render Sun/Moon based on currentTheme) so the button follows the
displayed scope theme; update references in PersistentControls and keep
useScopeTheme, theme, defaultLabel, setTheme names intact.
Comment on lines
+6
to
+13
| Object.defineProperty(window, 'localStorage', { | ||
| value: { | ||
| getItem: vi.fn(), | ||
| setItem: vi.fn(), | ||
| removeItem: vi.fn(), | ||
| clear: vi.fn() | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether other tests also redefine the same globals (risk of collision).
rg -n -C2 "Object\.defineProperty\(window,\s*'localStorage'|Object\.defineProperty\(window,\s*'matchMedia'|vi\.stubGlobal\(\s*'localStorage'|vi\.stubGlobal\(\s*'matchMedia'" --type=ts --type=tsxRepository: raystack/apsara
Length of output: 87
🏁 Script executed:
# First, verify the file exists and check the actual code at the specified lines
cat -n packages/raystack/components/theme-provider/__tests__/theme-scope.test.tsx | head -40Repository: raystack/apsara
Length of output: 1525
🏁 Script executed:
# Search for Object.defineProperty and vi.stubGlobal with corrected syntax
rg -n "Object\.defineProperty\(window|vi\.stubGlobal" --type ts packages/ -A 3 -B 1Repository: raystack/apsara
Length of output: 2190
🏁 Script executed:
# Specifically search for localStorage or matchMedia mocking in test files
rg -n "localStorage|matchMedia" --type ts packages/ | grep -i "defineProperty\|stubGlobal\|mock"Repository: raystack/apsara
Length of output: 4606
Add configurable: true to global property definitions to prevent test suite collisions.
Lines 6 and 15 define window.localStorage and window.matchMedia with Object.defineProperty but omit configurable: true. Since theme-provider.test.tsx redefines the same globals, running tests together will fail with "Cannot redefine property" on the second suite's initialization.
Add configurable: true to both definitions. Line 15's matchMedia already has writable: true, but localStorage at line 6 also needs writable: true.
Proposed fix
Object.defineProperty(window, 'localStorage', {
+ configurable: true,
+ writable: true,
value: {
getItem: vi.fn(),
setItem: vi.fn(),
removeItem: vi.fn(),
clear: vi.fn()
}
});
Object.defineProperty(window, 'matchMedia', {
+ configurable: true,
writable: true,
value: vi.fn().mockImplementation(query => ({📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Object.defineProperty(window, 'localStorage', { | |
| value: { | |
| getItem: vi.fn(), | |
| setItem: vi.fn(), | |
| removeItem: vi.fn(), | |
| clear: vi.fn() | |
| } | |
| }); | |
| Object.defineProperty(window, 'localStorage', { | |
| configurable: true, | |
| writable: true, | |
| value: { | |
| getItem: vi.fn(), | |
| setItem: vi.fn(), | |
| removeItem: vi.fn(), | |
| clear: vi.fn() | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/raystack/components/theme-provider/__tests__/theme-scope.test.tsx`
around lines 6 - 13, The test globals are defined with Object.defineProperty for
window.localStorage and window.matchMedia but they lack configurable:true (and
localStorage also lacks writable:true), causing "Cannot redefine property" when
other suites redefine them; update the Object.defineProperty call that sets
window.localStorage (the block creating getItem/setItem/removeItem/clear mocks)
to include configurable: true and writable: true, and update the
Object.defineProperty call that sets window.matchMedia to include configurable:
true so both globals can be redefined by other tests.
The migration of color/gray/accent/appearance declarations from `:root` to `[data-theme]`-only selectors was a silent break for consumers who render Apsara primitives outside a `<Theme>` wrapper — Storybook stories without a decorator, isolated tests, portaled roots, embedded widgets, and anyone importing a component standalone got an unstyled cascade. Add `:root` as a joint selector alongside the light-theme block in each file so the absence of a `data-theme` attribute resolves to the light defaults, matching the pre-PR behavior. Specificity stays balanced between `:root` and `[data-theme]` (both 0,0,1,0), and dark wins on source order when both apply, so scoped theming and overrides still work correctly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
rohanchkrabrty
requested changes
May 22, 2026
|
|
||
| /* [data-accent-color='indigo'] { */ | ||
| :root { | ||
| :root, |
Contributor
There was a problem hiding this comment.
Let's remove the default :root selector
| } | ||
|
|
||
| /* Native UI (form controls, scrollbars, text selection) follows the scoped theme */ | ||
| :root, |
Contributor
There was a problem hiding this comment.
Let's remove the default :root selector
| } | ||
|
|
||
| /* Light Theme Colors */ | ||
| :root, |
Contributor
There was a problem hiding this comment.
Let's remove the default :root selector
|
|
||
|
|
||
| [data-gray-color='gray'] { | ||
| :root, |
Contributor
There was a problem hiding this comment.
Let's remove the default :root selector
| [data-gray-color='gray'] { | ||
| :root, | ||
| [data-gray-color='gray'], | ||
| [data-theme='light'] { |
Contributor
There was a problem hiding this comment.
Why are we using this [data-theme='light'] here?
|
|
||
| [data-gray-color='gray'][data-theme='dark'] { | ||
| [data-gray-color='gray'][data-theme='dark'], | ||
| [data-theme='dark'] { |
Contributor
There was a problem hiding this comment.
Why are we using this [data-theme='dark'] here?
| */ | ||
|
|
||
| :root { | ||
| :root, |
Contributor
There was a problem hiding this comment.
Let's remove the default :root selector
Contributor
There was a problem hiding this comment.
- There is no point of branching out Stateless Scope. The main difference is storageKey which can be handled in a single component. Both the scope component can be merged and reduce logic dupe
- The expectation was that nested theme provider would only apply the overriden styles, but inherit rest of the stuff. But in provided example, if
accentColoris overriden, it's theme is not getting inherited from the root. - The choice of
useThemenot working in Stateless Scope is misleading. Local storage persistance shouldn't affect hook usage. Right now theuseThemeis ambiguous, from a call site a user cannot understand what will happen. It should ideally always update the nearestThemeContext.
a. If updating parent scope needs to be feature, then we can introduce something likeuseTheme({storageKey})and use that pattern to target outside ThemeContexts
| import { COLOR_SCHEMES } from './types'; | ||
|
|
||
| const colorSchemes = ['light', 'dark']; | ||
| const colorSchemes: string[] = [...COLOR_SCHEMES]; |
Contributor
There was a problem hiding this comment.
Can't we use the COLOR_SCHEMES directly?
Contributor
There was a problem hiding this comment.
All examples use ThemeProvider alias even tho it's deprecated
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 free
to 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.
Summary
Screen.Recording.2026-04-30.at.4.15.38.PM.mov
Scoped theming for Apsara — a nested
<Theme>creates an isolated themed region inside a page. Example:/examples/theme-overrides.What's new
Inherits from the parent by default. A nested
<Theme>with no props is invisible — children render exactly as they would without it. You only pay for what you override.Pick which axes to override. Pass any combination of theme (light/dark), accent color, gray, or style variant. Anything you don't pass keeps inheriting from the parent.
Remembers user choices when you give it a
storageKey. Toggle a region's theme and it sticks — survives reloads, syncs across tabs automatically. Clear it to fall back to inheriting again.Themeis the canonical name.ThemeProviderkeeps working as a deprecated alias; no migration required, but new code should useTheme.Components inside a region pick up the local theme. Buttons, callouts, shadows, native scrollbars, and the smooth theme-switch transition all follow the nearest scope — no extra plumbing in component code.
Test plan
pnpm testinpackages/raystack— 38 theme-provider tests pass./examples/theme-overrides— every mode has inline test steps and expected behavior.<div data-theme="dark">inside a light page — native UI (checkboxes, scrollbars), shadows, and accent tokens all follow the scope.