Connect: Add theme configuration by gzdunek · Pull Request #27788 · gravitational/teleport

gzdunek · 2023-06-13T09:30:15Z

Contributes to #27079

Enables Connect theme configuration. The available options are: light, dark, system (sync with OS).
The default is system.

The implementation turned to be slightly more complex than I thought, because of two Linux-related issues:

prefers-color-scheme is not updated when the system theme changes. There is a bug in Chromium related to to this. Apparently it was fixed in Chromium 114, but people are still reporting issues with that (which is interesting, I tested Chromium 114 on my Ubuntu VM and webpages seemed to respect the system theme). Probably there have be some work done on the Electron side, because I tested Electron 25 (Chromium 114) and it was not better than Electron 24.
The workaround that I added relies on nativeTheme updates that work correctly. These updates are then passed to the renderer via IPC and webContents.send.
On Fedora, nativeTheme is not updated at all 😓. It is a known issue. Because of that, the app is always rendered in the light mode and the user will have to switch do dark in the app settings if they won't like the light one. Theoretically, we could detect Fedora and use dark as a default value there, but I don't know if this is worth it, probably not.
I also took a look at vscode, but they don't have any smart fix for that.

(BTW I still have to work on the appearance of the tabs)

…ased on that

ravicious

I tested it and it works quite well, good job!

I'm not actually sure how we could use prefers-color-scheme here, it seems like nativeTheme is naturally a better fit for us here. 🤔

ravicious · 2023-06-15T13:40:34Z


+    nativeTheme.on('updated', () => {
+      window.webContents.send(
+        'main-process-subscribe-to-native-theme-update',


What do you think about changing this channel name to main-process-native-theme-update?

Seeing subscribe in this call makes it seem as if the main process was subscribing to something, but it's the other way around – the renderer process can call subscribeToNativeThemeUpdate on the main process client and the main process will send native theme updates to the renderer.

ravicious · 2023-06-15T13:48:32Z

+  // https://github.com/electron/electron/issues/33635#issuecomment-1502215450
+  const ctx = useAppContext();
+  const [activeTheme, setActiveTheme] = useState(
+    ctx.mainProcessClient.getNativeTheme() === 'dark' ? darkTheme : lightTheme


Let's pass an initializer function here to avoid calling the main process client on each render of ThemeProvider.

https://react.dev/reference/react/useState#avoiding-recreating-the-initial-state

Ah good catch, I didn't think of it.

ravicious · 2023-06-15T13:54:16Z

+  // https://github.com/electron/electron/issues/33635#issuecomment-1502215450
+  const ctx = useAppContext();
+  const [activeTheme, setActiveTheme] = useState(
+    ctx.mainProcessClient.getNativeTheme() === 'dark' ? darkTheme : lightTheme


Shouldn't this function be called shouldUseDarkColors instead and return a boolean? I feel like in its current state it conflates themeSource with the output of shouldUseDarkColors.

Good idea, it doesn't make much sense to convert boolean to string and then back to boolean (ctx.mainProcessClient.getNativeTheme() === 'dark'). I changed it also for the subscription.

ravicious · 2023-06-15T13:59:13Z

+        ? teletermDarkTheme
+        : teletermLightTheme;
+    return (
+      <MockAppContextProvider>


Instead of mocking the whole app context here, which has an effect on all stories, could we extract StaticThemeProvider out of teleterm's ThemeProvider which always uses the theme passed as a prop?

ravicious · 2023-06-15T14:05:47Z

Side note: This approach won't work well with custom themes as it couples the concept of a theme with the concept of automatically switching between a light theme and a dark theme.

VSCode solves this quite elegantly:

"workbench.colorTheme": "Alabaster", "workbench.preferredDarkColorTheme": "Default Dark+", "workbench.preferredLightColorTheme": "Alabaster", "window.autoDetectColorScheme": true,

Alas, it's good enough for now and I think we have a path to migrate to a more elaborate config scheme once we add custom themes – we'll have to treat theme: "system" as a special value but we should be able to get away with it.

Side note: This approach won't work well with custom themes as it couples the concept of a theme with the concept of automatically switching between a light theme and a dark theme.

Yeah, I'm aware of that, but I don't think we get custom themes anytime soon, so I would keep this simple for now.

gzdunek · 2023-06-16T09:04:19Z

I'm not actually sure how we could use prefers-color-scheme here, it seems like nativeTheme is naturally a better fit for us here. 🤔

If we have only light and dark theme, prefers-color-scheme is fine. It changes to prefers-color-scheme: dark when shouldUseDarkColors is true and to prefers-color-scheme: light when it's not. We could watch for changes using window.matchMedia and not have to add two IPCs :)

public-teleport-github-review-bot · 2023-06-16T09:48:12Z

@gzdunek See the table below for backport results.

Branch	Result
branch/v13	Failed

* Add a config item for the theme and adjust Electron's `nativeTheme` based on that * Listen to theme changes and update the app accordingly * React to theme changes in teleterm stories * Rename channel * Return boolean from handlers * Do not mock the whole app context in storybook * Fix linting issue * Fix typo (cherry picked from commit 95e6482)

* Connect: Adjust to the light theme (#27080) * Adjust Connect to light theme * Remove `clusters/*` element * Add terminal colors * Remove warning about using `false` as `color` * Add custom styling for `Toggle` * Fix light theme for file transfer, use the same border color for the drop area as for the input * Do not hardcode color in `CliCommand` * Use #000 as black * Convert rgba colors to be non-opaque * Fix two slightly incorrect colors (cherry picked from commit 1220470) * Connect: Add theme configuration (#27788) * Add a config item for the theme and adjust Electron's `nativeTheme` based on that * Listen to theme changes and update the app accordingly * React to theme changes in teleterm stories * Rename channel * Return boolean from handlers * Do not mock the whole app context in storybook * Fix linting issue * Fix typo (cherry picked from commit 95e6482) * Connect: Make tabs shadows look better (#27931) * Add bottom shadow for inactive tabs and inset for the active one * Add shadow for new tab item by using common styling * Adjust `KeyboardShortcutsPanel` to the light theme * Center Servers/Databases/Kubes navbar vertically between tabs and table by using the same margin values (8px) * Set title on the element that is dragged (cherry picked from commit 1220470)

gzdunek added 3 commits June 12, 2023 12:51

Add a config item for the theme and adjust Electron's nativeTheme b…

3f49619

…ased on that

Listen to theme changes and update the app accordingly

bacf6d1

React to theme changes in teleterm stories

1fe871b

gzdunek added the backport/branch/v13 label Jun 13, 2023

gzdunek requested review from avatus and ravicious June 13, 2023 09:30

github-actions Bot added size/sm ui labels Jun 13, 2023

github-actions Bot requested a review from rudream June 13, 2023 09:30

gzdunek removed the request for review from rudream June 13, 2023 09:31

zmb3 approved these changes Jun 13, 2023

View reviewed changes

avatus approved these changes Jun 13, 2023

View reviewed changes

public-teleport-github-review-bot Bot removed the request for review from ravicious June 13, 2023 15:50

ravicious self-requested a review June 14, 2023 09:08

ravicious approved these changes Jun 15, 2023

View reviewed changes

gzdunek added 3 commits June 16, 2023 09:50

Rename channel

2ad4d87

Return boolean from handlers

8bea57b

Do not mock the whole app context in storybook

e019fc1

gzdunek added 2 commits June 16, 2023 11:06

Fix linting issue

9d9bbbc

Fix typo

3bd12ba

gzdunek added this pull request to the merge queue Jun 16, 2023

Merged via the queue into master with commit 95e6482 Jun 16, 2023

gzdunek deleted the gzdunek/add-theme-configuration branch June 16, 2023 09:46

gzdunek mentioned this pull request Jun 26, 2023

[v13] Connect: Light theme #28277

Merged

ravicious mentioned this pull request Aug 24, 2023

[web] init history if null #30933

Merged

Conversation

gzdunek commented Jun 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ravicious left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gzdunek Jun 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gzdunek commented Jun 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

public-teleport-github-review-bot Bot commented Jun 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gzdunek commented Jun 13, 2023 •

edited

Loading

gzdunek Jun 16, 2023 •

edited

Loading

gzdunek commented Jun 16, 2023 •

edited

Loading