Copilot commented on code in PR #4284:
URL: https://github.com/apache/streampipes/pull/4284#discussion_r2976777816
##########
ui/src/app/core/components/streampipes/streampipes.component.ts:
##########
@@ -62,9 +62,16 @@ export class StreampipesComponent implements OnInit,
OnDestroy {
collapsed = this.collapseService.isCollapsed;
ngOnInit(): void {
- this.darkMode$ = this.currentUserService.darkMode$.subscribe(
- dm => (this.darkMode = dm),
- );
+ 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');
+ }
+ });
Review 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.
##########
ui/src/app/core/components/toolbar/toolbar.component.ts:
##########
@@ -145,13 +145,18 @@ export class ToolbarComponent
}
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);
+ });
Review 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.
##########
ui/src/app/profile/components/general/general-profile-settings.component.ts:
##########
@@ -110,6 +110,7 @@ export class GeneralProfileSettingsComponent
changeModePreview(value: boolean) {
this.currentUserService.darkMode$.next(value);
+ this.updateAppearanceMode();
}
Review 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 is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
