feat(ui-next): Adds appearance dialog with theme presets and custom theme support#6041
feat(ui-next): Adds appearance dialog with theme presets and custom theme support#6041dan-rukas wants to merge 30 commits into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
❌ Deploy Preview for ohif-dev failed. Why did it fail? →
|
dan-rukas
changed the title
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…n-rukas/Viewers into feat/appearance-dialog-simple
|
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:
📝 WalkthroughWalkthroughAdds active theme context and preset theme assets, an appearance modal UI and customization, app provider integration, and English localization for the new UI. ChangesAppearance Modal Theme Customization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
Pushed a round of fixes addressing the open review comment plus issues found in a deeper review pass: Custom CSS parsing (
Theme state validation
Modal UX / a11y
Header
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
platform/app/src/routes/WorkList/StudyListSettingsPopover.tsx (1)
43-54: ⚡ Quick winConsider using i18n for the menu label and checking menuTitle.
The label is hardcoded as
'Appearance', butViewerHeader.tsxusesAppearanceModal?.menuTitle ?? t('Header:Appearance')for its menu text. For better i18n support and consistency across menu contexts, consider:{ id: 'appearance', - label: 'Appearance', + label: AppearanceModal?.menuTitle ?? t('Header:Appearance'), onClick: () => { const AppearanceModal = customizationService.getCustomization('ohif.appearanceModal');Note: This pattern would also improve the About and User Preferences items in the same file.
🤖 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 `@platform/app/src/routes/WorkList/StudyListSettingsPopover.tsx` around lines 43 - 54, The menu item with id 'appearance' currently hardcodes label: 'Appearance'; update it to use i18n and the modal's menuTitle like ViewerHeader does by replacing the label with AppearanceModal?.menuTitle ?? t('Header:Appearance') (locate the onClick that calls customizationService.getCustomization('ohif.appearanceModal') and show(...) and compute AppearanceModal before building the menu item), and apply the same pattern to the About and User Preferences items for consistency.
🤖 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 `@platform/app/src/routes/WorkList/StudyListSettingsPopover.tsx`:
- Around line 43-54: The menu item with id 'appearance' currently hardcodes
label: 'Appearance'; update it to use i18n and the modal's menuTitle like
ViewerHeader does by replacing the label with AppearanceModal?.menuTitle ??
t('Header:Appearance') (locate the onClick that calls
customizationService.getCustomization('ohif.appearanceModal') and show(...) and
compute AppearanceModal before building the menu item), and apply the same
pattern to the About and User Preferences items for consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 300a232d-e853-4575-88c3-b309b79ff409
📒 Files selected for processing (1)
platform/app/src/routes/WorkList/StudyListSettingsPopover.tsx
sedghi
left a comment
sedghi
left a comment
There was a problem hiding this comment.
can we document how to enable/disable this theme in the dropdown, also if we are adding it in our master branch, let's document a migration if someone wants to turn it off
|
ideally should use a customization under default extension that is only on for netlify config that we deploy for our demo app. default js config is what most people use and edit so no changes there so that people don't get this theme unless they enable that customiztaion |
- Guard menu entries in ViewerHeader and StudyListSettingsPopover - Enable for Netlify demo config only - Fix i18n on StudyListSettingsPopover labels
|
Update: Provider registration moved to extension Moved What changed:
Why: App.tsx shouldn't inspect extension configs to decide whether to mount a specific provider. The extension that owns the theme module now owns the decision to register its provider. Zero footprint when theming is disabled is preserved — |
2 participants
Context
Adds a theme preset system to the Viewer with an Appearance dialog accessible from the header gear menu. Users can switch between 6 dark-mode color palettes — 3 tonal (full hue-shifted) and 3 neutral (achromatic surfaces with colored accent). Selections persist across sessions via localStorage and can be deep-linked via URL parameter.
This is a minimal-risk approach: master's
tailwind.cssis completely untouched, no new runtime dependencies are added, and the application renders identically to production when no preset is selected.Deploy preview: https://ohif-theme-apply.netlify.app/
Changes & Results
.theme-*class on<body>, persists to localStorage?theme=orchid,?theme=slate, etc. for deep-linking and testingAppearanceModaltranslation namespaceWhat's NOT included (intentional scope decisions):
tailwind.css— the foundational CSS layer is untouchedArchitecture
Provider Chain
A single new provider manages all theme state — preset selection, custom CSS parsing and injection, persistence, and URL parameter handling:
ActiveThemeProvideris added to the existing provider chain inApp.tsx. No ThemeProvider (next-themes) is needed because there is no mode toggle. The app stays dark-only, exactly as it is today.CSS Cascade
The override mechanism uses standard CSS custom property inheritance — no specificity tricks, no
!important, no layer manipulation:Two factors ensure presets always win:
themes.cssis outside@layer base, so it has higher priority thantailwind.css's@layer basetokens regardless of source order<body>override inherited values from<html>(:root)When the user selects "Default", the
.theme-*class is removed from<body>, and all variables fall back to:root. The application looks exactly as it does without this PR installed.Theme Preset Structure
Each preset is a complete, self-contained dark token set — all ~28 CSS variables are defined in every preset. No partial overrides.
Why complete sets instead of partial overrides:
.theme-*block and know exactly what the theme looks likeEach preset is defined in two places:
themes.css) —.theme-*block with all variables. This is what the browser uses at runtime.themes/*.json) —name,label, andcssVars.darkrecord. This drives the dropdown UI (label display, preset enumeration) and the theme creator tool.The JSON files intentionally contain only the 23 core design tokens (not chart, radius, or status tokens). Chart and status tokens inherit from the base theme in
tailwind.cssbecause they are currently identical across all presets. If a future preset needs different chart colors, they can be added to both the JSON and CSS at that time.New Files (14)
ui-next/src/contextProviders/ActiveThemeProvider.tsx.theme-*class management on<body>, localStorage + URL param, custom CSS parsing + injectionui-next/src/components/OHIFModals/AppearanceModal.tsxui-next/src/themes/themes.css.theme-*blocks, dark-only, outside@layer base)ui-next/src/themes/index.tsThemePresetinterface,themePresetsarray exportui-next/src/themes/orchid.jsonui-next/src/themes/arctic.jsonui-next/src/themes/verdant.jsonui-next/src/themes/midnight.jsonui-next/src/themes/slate.jsonui-next/src/themes/deep.jsonextensions/default/src/customizations/appearanceModalCustomization.tsxplatform/i18n/src/locales/en-US/AppearanceModal.jsonModified Files (7)
ui-next/src/contextProviders/index.tsui-next/src/components/OHIFModals/index.tsui-next/src/components/index.tsplatform/app/src/App.tsxplatform/i18n/src/locales/en-US/Header.jsonplatform/i18n/src/locales/en-US/index.jsextensions/default/src/ViewerLayout/ViewerHeader.tsxextensions/default/src/getCustomizationModule.tsxImportant notes
Why
tailwind.cssis untouchedtailwind.cssdefines every CSS variable in the application — it's the foundation of the entire UI. The only CSS this PR adds is in a newthemes.cssfile that only takes effect when a user actively selects a preset. Zero risk of regressions to existing styling.Why the custom theme injection writes to both
:rootand.darkThe custom theme paste injects CSS targeting both
:rootand.dark:On this branch there is no
.darkclass on<html>, so the.darkblock is currently inert. It exists as forward compatibility — if a dark/light mode toggle is added later (via next-themes, which adds.darkto<html>), the custom theme injection will work without changes.Why the custom theme paste feature exists
The paste feature lets users paste CSS variable overrides directly into OHIF to create fully custom themes without editing source files. Use cases include institutional branding, accessibility adjustments, and rapid theme prototyping. It also supports the theme creator tool workflow. Users can generate a palette externally and apply it to OHIF via paste.
All custom theme state: parsing, sanitization, injection, and persistence — is centralized in
ActiveThemeProvider. The modal is a thin UI layer that callsapplyCustomTheme()andclearCustomTheme()from context.Input sanitization: The
parseCssVarsfunction inActiveThemeProvidervalidates property names against--[a-zA-Z0-9-]+and strips{,}, and;from values before reassembling the CSS. This prevents CSS injection via crafted input that could break out of the:root {}block.Why
ActiveThemeProvidercleanup uses collect-then-removeThe
setActiveThemecallback and theuseEffectcleanup both need to removetheme-*classes fromdocument.body.classList. Both use a two-phase pattern: collect matching classes into an array first, then remove them. This avoids mutatingDOMTokenListduring iteration, which has implementation-dependent behavior and could skip elements if multipletheme-*classes were present.How to Add a New Preset
For a developer who wants to add a new theme preset:
Create the JSON —
platform/ui-next/src/themes/{name}.json:{ "name": "{name}", "label": "Tonal: {Display Name}", "cssVars": { "dark": { "background": "...", "foreground": "...", ...all 23 core tokens } } }Add CSS — Add a
.theme-{name}block tothemes.csswith all variables (core tokens + chart tokens)Register — Import the JSON in
themes/index.tsand add it to thethemePresetsarrayThe dropdown, persistence, URL parameter, and class management all work automatically — no other files need to change.
Testing
?theme=orchidURL parameter applies preset on load?theme=defaultURL parameter resets to default?theme=nonexistentis ignored (falls back to localStorage or default)}or{characters is sanitized (no CSS injection)yarn buildsucceeds with no TypeScript errorsChecklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
Summary by CodeRabbit
Greptile Summary
This PR adds a theme preset system to the OHIF Viewer — an Appearance dialog accessible from the header gear menu and the worklist settings popover. All theme state is centralized in a new
ActiveThemeProviderthat manages the.theme-*body class, localStorage persistence, URL parameter handling, and custom CSS injection with sanitization.ActiveThemeProviderinitialises theme from URL param or localStorage with well-guarded validation (stale keys and orphanedcustomstate both fall back todefault); the previously reported comment-injection and localStorage-corruption edge cases are addressed in this revision.themes.cssare placed outside@layer base, correctly overriding:roottokens via CSS proximity —tailwind.cssis entirely untouched.parseCssVarswhich strips comments, blocks, parentheses, and non----prefixed names before assembling the injected stylesheet.Confidence Score: 5/5
Safe to merge — changes are additive, the default app state is identical to production when no preset is selected, and the custom CSS sanitization pipeline is solid.
All theme mutations are confined to the new provider; tailwind.css is untouched. The initialization guards handle orphaned localStorage state. The CSS injection sanitizer rejects parentheses, braces, and comment sequences. No breaking changes to existing components or APIs.
No files require special attention.
Important Files Changed
Comments Outside Diff (1)
platform/ui-next/src/contextProviders/ActiveThemeProvider.tsx, line 478-488 (link)theme-customclass to<body>If
localStoragehasohif:theme = "custom"butohif:custom-theme-cssis missing (e.g. the key was set by a URL parameter that got persisted, the CSS key was cleared manually, or the user's storage was partially wiped), thenactiveThemeinitialises to'custom'whilecustomCssis''.The
useEffectbranch for this state iselse if (activeTheme !== 'default'), which addstheme-customto<body>. No.theme-customrule exists inthemes.css, so no styles change — but the class leaks into the DOM silently. Additionally, the Select'svalue="custom"matches no rendered<SelectItem>(the "Custom" item is conditionally rendered only whencustomCss || draftCssis truthy), leaving the dropdown visually blank or stuck.A safe fix is to guard the
activeTheme='custom'initialisation path: ifactiveThemeis'custom'butcustomCssis empty, fall back to'default'.Prompt To Fix With AI
Reviews (6): Last reviewed commit: "Added Appearance dialog to ui-next Study..." | Re-trigger Greptile