Feat:Improved dark theme implementation#4284
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves the app’s dark theme behavior and aligns more UI colors with theme-controlled CSS variables (closer to Angular Material 3 defaults), while also adjusting how appearance settings are applied and how new charts initialize their default colors.
Changes:
- Refactors Material theme overrides and dark-mode surface/background tokens in
sp-theme.scss. - Removes hardcoded light colors across multiple SCSS files in favor of
var(--color-...)tokens. - Updates appearance handling (profile + app shell) and sets chart default background/text colors to theme variables.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| ui/src/scss/sp/sp-theme.scss | Reworks Material theme overrides and defines explicit light/dark surface/background tokens. |
| ui/src/scss/sp/main.scss | Replaces several hardcoded colors with theme variables for better mode switching. |
| ui/src/app/profile/components/general/general-profile-settings.component.ts | Auto-persists appearance mode on toggle (no “save” step). |
| ui/src/app/profile/components/general/general-profile-settings.component.html | Removes “Save color schema” button from appearance section. |
| ui/src/app/editor/components/pipeline-element-icon-stand/pipeline-element-icon-stand-row/pipeline-element-icon-stand-row.component.scss | Removes hardcoded white background for draggable icon. |
| ui/src/app/core/components/toolbar/toolbar.component.ts | Applies dark/light mode class to html/body/overlay container. |
| ui/src/app/core/components/streampipes/streampipes.component.ts | Applies dark/light mode class based on darkMode$ subscription. |
| ui/src/app/core/components/iconbar/iconbar.component.scss | Adds dark-mode-specific hardcoded text/icon color for menu group headers/chevrons. |
| ui/src/app/core-ui/static-properties/static-mapping-unary/static-mapping-unary.component.scss | Uses theme background variable for select panels. |
| ui/src/app/connect/components/filter-toolbar/filter-toolbar.component.scss | Uses theme background variable for select panels. |
| ui/src/app/chart/components/chart-view/chart-view.component.ts | Initializes new widgets with theme-based background/text colors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| modifyAppearance(darkMode: boolean) { | ||
| if (darkMode) { | ||
| this.overlay.getContainerElement().classList.remove('light-mode'); | ||
| this.overlay.getContainerElement().classList.add('dark-mode'); | ||
| } else { | ||
| this.overlay.getContainerElement().classList.remove('dark-mode'); | ||
| this.overlay.getContainerElement().classList.add('light-mode'); | ||
| } | ||
| const targets = [ | ||
| document.documentElement, | ||
| document.body, | ||
| this.overlay.getContainerElement(), | ||
| ]; | ||
| const [addClass, removeClass] = darkMode | ||
| ? ['dark-mode', 'light-mode'] | ||
| : ['light-mode', 'dark-mode']; | ||
| targets.forEach(el => { | ||
| el.classList.remove(removeClass); | ||
| el.classList.add(addClass); | ||
| }); |
There was a problem hiding this comment.
modifyAppearance now updates html/body/overlay classes, but it is only called from within this component. If currentUserService.darkMode$ is changed elsewhere (e.g., profile appearance setting), modifyAppearance won’t run and body/overlay can remain in the previous mode while StreampipesComponent updates only the <html> class. Suggest subscribing to darkMode$ here (and keeping the form control in sync without re-emitting) or moving this DOM class toggling into a single shared service so all entry points apply the theme consistently.
| changeModePreview(value: boolean) { | ||
| this.currentUserService.darkMode$.next(value); | ||
| this.updateAppearanceMode(); | ||
| } |
There was a problem hiding this comment.
changeModePreview now persists the appearance change immediately. Two concrete issues with the current implementation: (1) external users are shown a banner that their settings can’t be changed, but this method will still issue the update request when they toggle; consider guarding against isExternalUser (or disabling the radio group) to avoid predictable failing requests, and (2) because this is now triggered on every toggle, add error handling (and possibly revert darkMode$ on failure) to avoid silently leaving the UI in a state that wasn’t saved server-side.
| this.darkMode$ = this.currentUserService.darkMode$.subscribe(dm => { | ||
| this.darkMode = dm; | ||
| if (dm) { | ||
| document.documentElement.classList.add('dark-mode'); | ||
| document.documentElement.classList.remove('light-mode'); | ||
| } else { | ||
| document.documentElement.classList.remove('dark-mode'); | ||
| document.documentElement.classList.add('light-mode'); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Theme class updates here only touch document.documentElement. Since ToolbarComponent.modifyAppearance also adds dark-mode/light-mode to document.body and the CDK overlay container, toggling darkMode$ outside the toolbar (e.g., from the profile appearance setting) can leave body/overlay with the previous mode, causing dialogs/overlays (and any CSS variables bound to .dark-mode/.light-mode) to render with the wrong theme. Consider centralizing theme application in a shared service, or update the same targets (html/body/overlay container) from this subscription and remove/keep the other logic accordingly so they cannot get out of sync.
Functionality changes:
-removed "save color schema" button in profile for more intuitive handling
-all chart types (table, pie, heatmap etc.) have default color on initialization (before it was on custom) to produce better visualization
Appearance change to mostly match Angular Material 3 standard (removed the use of manual light-dark() implementation):
-changed background to dark grey (before black) for better contrast
-removed any hardcoded colors that don't respect theme switching
-exception: hard coded color of the menu-group-title (words "Analytics", "Visualization", "Management"), since they are on a green background and difficult to read with the default color schema
The first and last 3 pictures are the before, all others are the after
