fix(colors): Replace hardcoded colors with theme tokens#6040
Open
dan-rukas wants to merge 14 commits into
Open
fix(colors): Replace hardcoded colors with theme tokens#6040dan-rukas wants to merge 14 commits into
dan-rukas wants to merge 14 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? →
|
📝 WalkthroughWalkthroughSVG icons were converted from hardcoded colors to ChangesIcon and Component Color Theming
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
platform/ui-next/src/components/Icons/Sources/Upload.tsx (1)
4-10:⚠️ Potential issue | 🟠 Major | ⚡ Quick winForward icon props to the
<svg>root.
propsare accepted but never applied. Line 10 should include{...props}so callerclassName/style/ARIA propagate; otherwisecurrentColortheming can’t be driven from usage sites.Proposed fix
export const Upload = (props: IconProps) => ( <svg width="18" height="18" viewBox="0 0 18 18" xmlns="http://www.w3.org/2000/svg" + {...props} >Also applies to: 13-13
🤖 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/ui-next/src/components/Icons/Sources/Upload.tsx` around lines 4 - 10, The Upload component accepts IconProps but doesn't forward them to the SVG root; update the Upload functional component so the <svg> element spreads the incoming props (e.g., {...props}) to ensure className, style, aria attributes and theming like currentColor propagate; locate the Upload declaration and apply the props spread on the root <svg> element (and similarly ensure any other icon components mentioned also forward their props).
🤖 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.
Outside diff comments:
In `@platform/ui-next/src/components/Icons/Sources/Upload.tsx`:
- Around line 4-10: The Upload component accepts IconProps but doesn't forward
them to the SVG root; update the Upload functional component so the <svg>
element spreads the incoming props (e.g., {...props}) to ensure className,
style, aria attributes and theming like currentColor propagate; locate the
Upload declaration and apply the props spread on the root <svg> element (and
similarly ensure any other icon components mentioned also forward their props).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 83f38a72-6558-4d61-bc6f-cd459b6866fb
📒 Files selected for processing (32)
extensions/cornerstone/src/components/DicomUpload/DicomUploadProgressItem.tsxextensions/cornerstone/src/components/TrackingStatus/TrackingStatus.tsxextensions/default/src/Components/DataSourceConfigurationModalComponent.tsxextensions/default/src/DicomTagBrowser/DicomTagBrowser.cssextensions/default/src/utils/colorPickerDialog.cssplatform/app/src/routes/NotFound/NotFound.tsxplatform/ui-next/src/assets/styles.cssplatform/ui-next/src/components/Icons/Sources/Alert.tsxplatform/ui-next/src/components/Icons/Sources/AlertOutline.tsxplatform/ui-next/src/components/Icons/Sources/ContentNext.tsxplatform/ui-next/src/components/Icons/Sources/ContentPrev.tsxplatform/ui-next/src/components/Icons/Sources/Helpers.tsxplatform/ui-next/src/components/Icons/Sources/IconTransferring.tsxplatform/ui-next/src/components/Icons/Sources/IllustrationNotFound.tsxplatform/ui-next/src/components/Icons/Sources/Layout.tsxplatform/ui-next/src/components/Icons/Sources/LoadingSpinner.tsxplatform/ui-next/src/components/Icons/Sources/NotificationInfo.tsxplatform/ui-next/src/components/Icons/Sources/NotificationWarning.tsxplatform/ui-next/src/components/Icons/Sources/Plus.tsxplatform/ui-next/src/components/Icons/Sources/Search.tsxplatform/ui-next/src/components/Icons/Sources/Show.tsxplatform/ui-next/src/components/Icons/Sources/SocialGithub.tsxplatform/ui-next/src/components/Icons/Sources/StatusAlert.tsxplatform/ui-next/src/components/Icons/Sources/StatusLocked.tsxplatform/ui-next/src/components/Icons/Sources/StatusTracking.tsxplatform/ui-next/src/components/Icons/Sources/StatusUntracked.tsxplatform/ui-next/src/components/Icons/Sources/Tools.tsxplatform/ui-next/src/components/Icons/Sources/Upload.tsxplatform/ui-next/src/components/LayoutSelector/LayoutSelector.tsxplatform/ui-next/src/components/Onboarding/Onboarding.cssplatform/ui-next/src/components/ProgressLoadingBar/ProgressLoadingBar.cssplatform/ui-next/src/components/Sonner/Sonner.tsx
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
11
to
+14
| > | ||
| <path | ||
| d="M227.792 11.7344V16H226.702V8.90909H227.755V10.017H227.847C228.013 9.65696 228.266 9.36766 228.604 9.14915C228.943 8.92756 229.38 8.81676 229.915 8.81676C230.395 8.81676 230.816 8.91525 231.176 9.11222C231.536 9.30611 231.816 9.60156 232.016 9.99858C232.216 10.3925 232.316 10.8911 232.316 11.4943V16H231.226V11.5682C231.226 11.0111 231.082 10.5772 230.792 10.2663C230.503 9.95241 230.106 9.79545 229.601 9.79545C229.254 9.79545 228.943 9.87086 228.669 10.0217C228.398 10.1725 228.184 10.3925 228.027 10.6818C227.87 10.9711 227.792 11.322 227.792 11.7344Z" | ||
| fill="white" | ||
| fill="hsl(var(--foreground))" |
Contributor
There was a problem hiding this comment.
The PR description explicitly lists OHIFLogo variants under "Intentionally hardcoded (brand/regulatory marks)" and says they were not changed, but OHIFLogoHorizontal has all its text path fills changed from white to hsl(var(--foreground)) and its OHIF-grid icon fills from #358CFD to hsl(var(--primary)). On a light theme, --foreground will likely be dark, which would make the logo text invisible against a light header background (or vice versa). If the intent was to keep this logo hardcoded for brand consistency, these changes should be reverted. If the change is intentional, the PR description and the "Not Changed" section should be updated.
Prompt To Fix With AI
This is a comment left during a code review.
Path: platform/ui-next/src/components/Icons/Sources/OHIFLogoHorizontal.tsx
Line: 11-14
Comment:
**Logo text colour now follows theme — inconsistent with stated policy**
The PR description explicitly lists `OHIFLogo` variants under "Intentionally hardcoded (brand/regulatory marks)" and says they were not changed, but `OHIFLogoHorizontal` has all its text path fills changed from `white` to `hsl(var(--foreground))` and its OHIF-grid icon fills from `#358CFD` to `hsl(var(--primary))`. On a light theme, `--foreground` will likely be dark, which would make the logo text invisible against a light header background (or vice versa). If the intent was to keep this logo hardcoded for brand consistency, these changes should be reverted. If the change is intentional, the PR description and the "Not Changed" section should be updated.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Member
Author
There was a problem hiding this comment.
lol - updated the PR message
sedghi
approved these changes
Jun 11, 2026
sedghi
left a comment
sedghi
left a comment
Member
There was a problem hiding this comment.
Thanks, can you update the docs migration that we have changed our icons to do these
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.
Context
Replaces hardcoded hex color values across CSS files, icon SVGs, and usage sites so the UI responds to theme changes. Previously, ~40 hex values were frozen to the default OHIF dark palette regardless of any theme override.
32 files changed — 126 insertions, 129 deletions.
currentColor,hsl(var(--accent)), ortransparentChanges & Results
CSS / Component Changes (6 files)
LayoutSelector.tsx — Grid cell unhovered state
bg-[#04225b]→bg-accentOnboarding.css — Shepherd.js onboarding buttons
!bg-[#348cfd]→!bg-primary!text-[#348cfd]→!text-primarystyles.css — Custom scrollbar
scrollbar-color: #173239→hsl(var(--neutral) / 0.5)(matches ScrollArea and ImageScrollbar convention)background-color: #041c4athat overrode existing@apply bg-popoveron webkit scrollbar thumbProgressLoadingBar.css — Loading bar track
background-color: #091731→hsl(var(--muted))DicomTagBrowser.css — DICOM tag table
color: #ffffff→hsl(var(--foreground))border-top: 1px solid #ddd(cleaner without row separators)color: '#20A5D6'(quoted hex was invalid CSS — never applied)colorPickerDialog.css — Segmentation color picker
background: #090c29→transparent(inherits from parent dialog)box-shadow: noneto remove react-color's default drop shadowIcon SVG Changes (19 files)
A. Single-color →
currentColor(9 files, 10 icons)One hex color per icon replaced with
currentColor. Icons now inherit parent text color.stroke="#5ACCE6"currentColorfill="#FFFFFF"currentColorfill="#FFFFFF"currentColorstroke="#5ACCE6"currentColorstroke="#358CFD"×11currentColorstroke="#348CFD"currentColorstroke="#348CFD"currentColorstroke="#348CFD"×5currentColorfill="#FFFFFF"currentColorstroke="#348CFD"currentColorB. Two-tone →
currentColor+ contrast marks stay (5 files)Theme-colored shape becomes
currentColor. Internal contrast marks (#FFF,#000) stay hardcoded — they provide the visual detail against the colored background.fill="#B70D11"→currentColorstroke="#FFF"(exclamation mark)fill="#0944B3"→currentColorstroke="#FFF"(info symbol)fill="#F3DC43"+stroke="#F3DC43"→currentColorstroke="#000"(exclamation)stroke="#5ACCE6"+fill="#5ACCE6"→currentColorstroke="#000"(checkmark)fill="#348CFD"(ring, 25% opacity) +fill="#5ACCE6"(arc) → bothcurrentColorLoadingSpinner also fixes a minor issue: destructures
classNamefrom props to avoidundefinedin the className template string.C. Layout thumbnails →
hsl(var(--accent))(1 file, 4 icons)Active pane fill in layout selector thumbnails. Uses
--accentCSS variable directly as an SVG fill attribute.fill="#263A71"→fill="hsl(var(--accent))"fill="#263A71"→fill="hsl(var(--accent))"fillfrom<g>to child<rect>, →fill="hsl(var(--accent))"fill="#263A71"→fill="hsl(var(--accent))"Why
hsl(var(--accent))instead ofcurrentColor: these thumbnails have both outlined panes (stroke) and one filled pane (the active/highlighted one).currentColorwould make the filled pane match the stroke color. The accent token gives it a distinct highlight that tracks the theme.D. Status badge dark fills →
transparent(3 files)Status badges had
fill="#0D0E24"(near-black) as a background fill inside their ring — the old dark theme color baked into the icon. Replaced withtransparentso the badge becomes a ring-only indicator that works on any background.fill="#0D0E24"→transparent,stroke="#7BB2CE"→currentColorfill="#0D0E24"→transparent,stroke="#7BB2CE"→currentColorfill="#0D0E24"→transparent(stroke was alreadycurrentColor)E. Tools gear →
hsl(var(--primary))(1 file, 2 icons)The gear icon on ToolLayout and ToolLayoutDefault uses
--primaryto stay visually distinct from the grid lines in the same icon.stroke="#348CFD"→stroke="hsl(var(--primary))"stroke="#348CFD"→stroke="hsl(var(--primary))"F. Helpers →
hsl(var(--accent))+currentColor(1 file, 3 icons)Segmentation helper icons (HelperCombineSubtract, HelperCombineIntersect, HelperCombineAdd) had three types of hardcoded color:
fill="#1A3F7E"(overlap region)fill="hsl(var(--accent))"stroke="white"(dashed borders)stroke="currentColor"fill="white"(labels)fill="currentColor"Usage-Site Updates (7 files)
Most icons inherit appropriate color from their parent context. These sites needed explicit color classes because the parent doesn't provide the right color.
text-highlighttext-highlighttext-highlighttext-destructivetext-highlighttext-primaryon parent divtext-highlighttext-destructiveNot Changed (intentional)
Legacy-only icons — reverted to hardcoded (components being replaced):
platform/ui/Select.tsxplatform/ui/EmptyStudies.tsxThese were converted then explicitly reverted because their only consumers are legacy
platform/uicomponents that are being phased out. Converting the icons without updating those legacy consumers would break their appearance.Icons already correct (no changes needed):
currentColorIntentionally hardcoded (brand/regulatory marks):
OHIFLogo, OHIFLogoColorDarkBackground, LoadingOHIFMark — brand logos(now included)InvestigationalUse — regulatory badge(now included)Deferred (separate scope):
Testing
CSS / Component Tests
localStorage.removeItem('shownTours'), refresh, open study → verify button colors/localimport → verify track colorIcon Tests
text-highlight), StatusAlert/Locked/Untracked rings visible, interiors transparenttext-destructive/nonexistent→ 404 illustration in primary blueRegression
text-blackparent)Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
Summary by CodeRabbit
Greptile Summary
This PR replaces hardcoded hex colors across ~40 files (CSS, SVG icons, and React components) with CSS variable tokens so the UI responds to theme changes. The approach is systematic: single-color icons use
currentColor, accent-filled shapes usehsl(var(--accent)), and usage sites add explicittext-*classes where parent inheritance is insufficient.currentColorfor single-color icons,hsl(var(--accent/primary))for accent fills that must stay distinct from stroke color, andtransparentfor status badge ring backgrounds.LoadingSpinneralso fixes a latent bug where spreading{...props}onto the SVG element overrode the computedclassNamecontaining the animation classes.Confidence Score: 4/5
Safe to merge after confirming the OHIFLogoHorizontal and InvestigationalUse changes are intentional — both files are listed in the PR as not changed but are changed in the diff, and they carry brand/regulatory significance.
The bulk of the change is straightforward and low-risk: replacing hardcoded hex values with CSS variable tokens. Two files stand out: OHIFLogoHorizontal and InvestigationalUse are both explicitly described as intentionally hardcoded in the PR description, yet both have their colors converted to theme tokens in the actual diff. OHIFLogoHorizontal's text turning dark on a light theme could make the logo illegible; InvestigationalUse is a regulatory disclaimer badge where legibility requirements may demand stable, high-contrast colors.
platform/ui-next/src/components/Icons/Sources/OHIFLogoHorizontal.tsx and platform/ui-next/src/components/Icons/Sources/InvestigationalUse.tsx — both are listed as intentionally unchanged in the PR description but have been converted in the diff.
Important Files Changed
whitetohsl(var(--foreground))and OHIF grid icon fills from#358CFDtohsl(var(--primary)); the PR description lists OHIF logos as intentionally hardcoded brand marks but this file is changed anyway.classNamefrom props and uses nullish coalescing, fixing a bug where spreading{...props}would clobber theanimate-spinclass whenclassNamewas undefined.#1A3F7E→hsl(var(--accent)), allwhitestrokes/fills →currentColor; consistent and complete.#263A71→hsl(var(--accent)); fill correctly moved from<g>to child<rect>in LayoutAdvanced3DOnly.text-highlightandtext-destructiveclasses to ensure correct colors.background-color: #041c4aoverrides removed from webkit scrollbar thumb rules.transparentand default box-shadow removed to inherit from parent dialog.Comments Outside Diff (1)
platform/ui-next/src/components/Icons/Sources/InvestigationalUse.tsx, line 36-45 (link)The PR description lists
InvestigationalUseunder "Intentionally hardcoded (brand/regulatory marks)" and says it was not changed, but the file is changed: the background fill, ring stroke, and figure stroke have all been moved tohsl(var(--muted)),hsl(var(--input)), andhsl(var(--primary)). Since this is a regulatory disclaimer icon that must meet specific legibility standards on any background, it warrants explicit confirmation that theme-adapting its colors satisfies those requirements, or the changes should be reverted per the stated policy.Prompt To Fix With AI
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix All With AI
Reviews (5): Last reviewed commit: "Updated study list non-UI art" | Re-trigger Greptile