Add dark theme support by Akm0d · Pull Request #864 · erwindon/SaltGUI

Akm0d · 2026-04-14T14:34:21Z

erwindon · 2026-04-14T18:52:52Z

@Akm0d
impressive!

I compared Firefox-in-normal-mode side-by-side with Chrome-in-dark-mode.

The new code has a few deviations that must be handled before allowing it (in random order):

The main menu bar still uses black text. That makes it unreadable on the new background. I would expect that text to be made white(ish);
The tooltips used to have transparency, that is now gone;
The color of the values in the Status column (when not replaced by an IP-number) should be green (for accepted), or red (for offline), other colours are used on the Keys screen for accepted(green)/rejected(blue)/others;
Some screens have an embedded button images in the status text. See Keys screen. The color of that button should match other buttons;
There are 14 problems reported by "npm run stylelint". These must be corrected.
In a state.apply job output various elements get an non-default color. e.g. The minion name gets an overall success/fail indication, should e.g be bright green.

I'm too lazy to provide screenshots now for these items. Feel free to ask for that, I'll then add them.

For new features, at least a few lines must be added to the documentation.
It should at least describe that dark-mode happens when requested by the browser settings.

Is it commonly expected that people can switch this on/off in the configuration file in an application?
In that case we need to introduced a setting for it in /etc/salt/master too.
And as common make that setting visible on the Settings page.

I missed the relation with Home Assistant, but you already fixed that by removing it from the title :-)
But the filenames still contain the prefix ha-.
Can you explain the relation with Home Assistant?

You took care not to touch any existing code.
But with all the new CSS rules, here will be more work for the maintainer (that's me!).
That is a bit of a blocker.
Can this change be re-factored so that:

first: all current colors are behind symbolic color-names
second: dark mode is nothing more than selecting another table of symbolic color-names

that would make the situation much more symmetrical.

Akm0d · 2026-04-14T22:29:05Z

@Akm0d impressive!

I compared Firefox-in-normal-mode side-by-side with Chrome-in-dark-mode.

The new code has a few deviations that must be handled before allowing it (in random order):
* The main menu bar still uses black text. That makes it unreadable on the new background. I would expect that text to be made white(ish);

* The tooltips used to have transparency, that is now gone;

* The color of the values in the Status column (when not replaced by an IP-number) should be green (for accepted), or red (for offline), other colours are used on the Keys screen for accepted(green)/rejected(blue)/others;

* Some screens have an embedded button images in the status text. See Keys screen. The color of that button should match other buttons;

* There are 14 problems reported by "npm run stylelint". These must be corrected.

* In a state.apply job output various elements get an non-default color. e.g. The minion name gets an overall success/fail indication, should e.g be bright green.
I'm too lazy to provide screenshots now for these items. Feel free to ask for that, I'll then add them.

For new features, at least a few lines must be added to the documentation. It should at least describe that dark-mode happens when requested by the browser settings.

Is it commonly expected that people can switch this on/off in the configuration file in an application? In that case we need to introduced a setting for it in /etc/salt/master too. And as common make that setting visible on the Settings page.

I missed the relation with Home Assistant, but you already fixed that by removing it from the title :-) But the filenames still contain the prefix ha-. Can you explain the relation with Home Assistant?

You took care not to touch any existing code. But with all the new CSS rules, here will be more work for the maintainer (that's me!). That is a bit of a blocker. Can this change be re-factored so that:
* first: all current colors are behind symbolic color-names

* second: dark mode is nothing more than selecting another table of symbolic color-names
that would make the situation much more symmetrical.

Thanks for the feedback! And thank you for such a thorough review. I'll address the things you mentioned right away.

I'm using SaltGUI in my home-assisstant add-on so that's where the naming came from -- but it should be more generic for a PR to upstream.

erwindon · 2026-04-14T23:33:18Z

FYI: I've accepted the sonarcloud issue on this._registerPage(Router.loginPage = new LoginPage(this));

erwindon · 2026-04-15T00:16:25Z

A generic review comment: please keep the code changes in this PR limited to the introduction of the dark-mode.
If anything bothers you in supporting the HA environment, or anything else, please leave that to a separate PR.
The main reason behind this rule is that I must fully understand all changes in a PR because I have to maintain it after the PR merge. It also leads to smaller changes that behave better in a future git bisect.

erwindon · 2026-04-15T15:46:47Z

@Akm0d
I one of my reviews of your branch, I noticed that the GitHub logo on the login-panel looked bad with the dark background.
Then I found out that I was still using the old logo from the 2012-2015 era.
I just updated the logo on the master branch.
The actual image looks the same (on the scale that we use it), but it now also uses the transparent background.

Akm0d · 2026-04-15T16:11:38Z

erwindon · 2026-04-15T21:32:26Z

@Akm0d
The unit test "test documentation external link conversion" fails because of the migration with css classes.
Here is the fix, can you please include it in your PR?

--- a/tests/unit/Output.test.js
+++ b/tests/unit/Output.test.js
@@ -433,7 +433,7 @@ describe("Unittests for Output.js", () => {
     OutputDocumentation.addDocumentationOutput(container, output);
     assert.isTrue(
       container.innerHTML.includes(
-        "<a href='https://www.freedesktop.org/software/systemd/man/systemd-run.html' target='_blank' rel='noopener'><span style='color: yellow'>systemd-run(1)</span></a>"));
+        "<a href='https://www.freedesktop.org/software/systemd/man/systemd-run.html' target='_blank' rel='noopener'><span class='doc-inline-highlight'>systemd-run(1)</span></a>"));
 
     done();

you can run all tests using just sh runtests.sh. This runs the same tests as the GitHub pipeline attached to this project.

erwindon · 2026-04-15T21:41:11Z

This is the CommandBox (click on >_).
The text "Waiting for command…" (below the Run command button) is only barely visable.

erwindon · 2026-04-15T21:44:49Z

In page Nodegroups, the nodegroups are divided by a double-line.

Note that this page is only visible when you actually have nodegroups defined in the /etc/salt/master file.

In light:

In dark:

I think they need to be more subtle.

Akm0d · 2026-04-16T23:18:45Z

erwindon · 2026-04-16T23:46:14Z

left = master branch, right = dark_mode branch
colors are odd in all 4 controls.

the GitHub logo is only different because I already updated it on the main branch.
can you please rebase dark_mode to master?

I'm currently ignoring all issues except styling. once that is done we'll go through the switching logic and so...

erwindon · 2026-04-17T00:18:35Z

converted PR to draft because you are working on it (no hidden meaning here)

…e text contrast and subtle nodegroup divider

erwindon · 2026-04-17T00:22:10Z

the PR stays in draft until all issues are agreed on.

erwindon · 2026-04-17T00:40:13Z

question:
why do we need a setting saltgui_theme: ... in SaltGUI?
Isn't it sufficient to just follow the browser settings and just look at that?

Akm0d · 2026-04-17T00:43:49Z

question: why do we need a setting saltgui_theme: ... in SaltGUI? Isn't it sufficient to just follow the browser settings and just look at that?

It would leave the potential for more themes besides light/dark to be added later. But I can remove it to just follow the browser setting for light/dark if that's preferred

erwindon · 2026-04-17T01:00:41Z

It would leave the potential for more themes besides light/dark to be added later. But I can remove it to just follow the browser setting for light/dark if that's preferred

sorry for all the work on that bit, but I want to keep SaltGUI a simple as possible.
for the benefit of the users, the contributors and the maintainer/myself.
so yes, please remove the in-app settings.

erwindon · 2026-04-17T01:36:42Z

I think we need this to fix the double-border-colour in the NodeGroups panel:

diff --git a/saltgui/static/scripts/panels/Nodegroups.js b/saltgui/static/scripts/panels/Nodegroups.js
index 4506571f..c141207a 100644
--- a/saltgui/static/scripts/panels/Nodegroups.js
+++ b/saltgui/static/scripts/panels/Nodegroups.js
@@ -308,7 +308,7 @@ export class NodegroupsPanel extends Panel {
 
   _addNodegroupRow (pNodegroup, pAllNodegroups) {
     const tr = Utils.createTr("no-search", null, "ng-" + pNodegroup);
-    tr.style.borderTop = "4px double #ddd";
+    tr.style.borderTop = "4px double var(--color-border-default)";
 
     const menuTd = Utils.createTd();
     tr.dropdownmenu = new DropDownMenu(menuTd, "smaller");

erwindon · 2026-04-17T01:44:26Z

With the update of the GitHub logo on the login panel, I added both the light and dark images to SaltGUI.
But so far we only used the black image.
Feel free to swap the images in the dark theme.

erwindon · 2026-04-17T02:09:53Z

variable configuredTheme is now unused.
variable root.dataset.themePreference is now unused (set once).

… theme cleanup

erwindon · 2026-04-17T09:11:10Z

Address final review nits: [...]

The login panel needs some adjustments to bring back the original light mode. and to let its dark mode be consistent with that.

Also, after the graphical bits, we'll focus on the code bits...

erwindon · 2026-04-17T09:22:29Z

+      attributeFilter: ["data-theme"],
+      attributes: true,
+    });
    img.style = "width: 1em; margin-right: 5px";


took me a few seconds to understand why this is 9 lines of code instead of just 2.
can you add a comment just before "new MutationObserver" with something like "// follow theme changes while the panel is already shown"

erwindon · 2026-04-17T09:27:55Z

+      Router.loginPage.login.bootstrapSession();
+    }
+
    const hash = window.location.hash.replace(/^#/, "");


Is this needed for the datk-theme support in SaltGUI? Or is this only needed to support HA?
In that case please move the change to a next PR.

erwindon · 2026-04-17T09:28:18Z

+    this.bootstrapSession();
+
+    // allow the success message to be seen
+    window.setTimeout(() => {
+      // erase credentials since we don't do page-refresh
+      this.usernameField.value = "";
+      this.passwordField.value = "";
+      if (Utils.getStorageItem("session", "login_response") !== null) {
+        // we might have been logged out in this first second
+        // e.g. when clock between client and server differs more than the session timout
+        const urlParams = new URLSearchParams(window.location.search);
+        if (urlParams.get("page")) {
+          // a redirect page is specified
+          const params = {};
+          for (const pair of urlParams.entries()) {
+            params[pair[0]] = pair[1];
+          }
+          const page = params["page"];
+          delete params["page"];
+          this.router.goTo(page, params);
+        } else {
+          this.router.goTo("");
+        }
+      }
+    }, 1000);
+
+  }
+
+  bootstrapSession () {


Is this needed for the datk-theme support in SaltGUI? Or is this only needed to support HA?
In that case please move the change to a next PR.

erwindon · 2026-04-17T09:38:44Z


+## Theme
+SaltGUI follows the browser color-scheme preference.
+When SaltGUI is embedded, it also uses theme hints from the parent frame when those are available.


my preference is to move all HA related additions/variations to a next PR.
the second line fits that.
this is just to show the separation between the requirements.

…theme detection (HA-specific), simplify login logo, remove ingress bootstrapSession

sonarqubecloud · 2026-04-18T02:36:37Z

Quality Gate passed

Issues
0 New issues
3 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

erwindon · 2026-04-18T11:21:59Z

@@ -0,0 +1,20 @@
+(function initializeTheme () {
+  const context = globalThis;


globalThis is from ES2020, but SaltGUI promises at most ES6/ES2015.
In upcoming change #865, I will set the new policy to ES2020.
Until that time, can you provide an alternative?

erwindon · 2026-04-18T11:24:42Z

Are you still working on Login.js?

Akm0d changed the title ~~Add Home Assistant dark theme support~~ Add dark theme support Apr 14, 2026

erwindon self-assigned this Apr 14, 2026

erwindon added enhancement work-in-progress new-feature labels Apr 14, 2026

erwindon marked this pull request as draft April 14, 2026 17:03

erwindon assigned Akm0d and unassigned erwindon Apr 14, 2026

erwindon reviewed Apr 14, 2026

View reviewed changes

Comment thread saltgui/static/scripts/panels/Login.js Outdated