Connect: Adjust to the light theme #27080

gzdunek · 2023-05-29T17:12:18Z

Contributes to #27079
e https://github.com/gravitational/teleport.e/pull/1522

Our theme was used incorrectly in some places. This led to some visual issues when applied the light theme. This PR fixes that.
The PR also adds our own terminal theme, which we have worked on with @roraback (it will be reused by the WebUI, fyi @rudream). I think this the most important part of the PR, I have tested the terminal in various cases and I think the colors look good, but I would like to hear other's voices.

To test the changes, please manually replace theme.colors in theme.ts with the values from the light theme (also please remember about changing the theme name to light - it is hardcoded in some places, unfortunately).

Patch

diff --git a/web/packages/teleterm/src/ui/ThemeProvider/theme.ts b/web/packages/teleterm/src/ui/ThemeProvider/theme.ts
index 4a89876af2..91337a3716 100644
--- a/web/packages/teleterm/src/ui/ThemeProvider/theme.ts
+++ b/web/packages/teleterm/src/ui/ThemeProvider/theme.ts
@@ -17,13 +17,13 @@ limitations under the License.
 import { fonts } from 'design/theme/fonts';
 import typography, { fontSizes, fontWeights } from 'design/theme/typography';
 import { sharedStyles } from 'design/theme/sharedStyles';
-import { darkTheme } from 'design/theme';
+import { lightTheme } from 'design/theme';
 
 const sansSerif = 'system-ui';
 
 const theme = {
-  name: 'dark',
-  colors: darkTheme.colors,
+  name: 'light',
+  colors: lightTheme.colors,
   typography,
   font: sansSerif,
   fonts: {
@@ -32,8 +32,8 @@ const theme = {
   },
   fontWeights,
   fontSizes,
-  space: darkTheme.space,
-  borders: darkTheme.borders,
+  space: lightTheme.space,
+  borders: lightTheme.borders,
   radii: [0, 2, 4, 8, 16, 9999, '100%'],
   regular: fontWeights.regular,
   bold: fontWeights.bold,

An app setting and the logic that will switch between the themes will be added in another PR.

Btw, I think we should work more on that shadow below the tabs, IMO it was okay on the dark theme, but on the light it looks worse :( I have some ideas, I will send PR after a discussion with the design team.

ravicious

Looks pretty good! 👍

I'm looking forward to the tab bar changes!

ravicious · 2023-05-30T14:34:12Z

web/packages/design/src/theme/lightTheme.ts

The input text in file upload path is invisible.

Thanks for catching this.
I also changed "Drag your files here" border to match the input's border.

ravicious · 2023-05-30T14:40:37Z

web/packages/design/src/theme/lightTheme.ts

I feel like the active tab could have a little more contrast. It's rather hard to immediately spot which tab is active on my old full HD monitor.

I think it's sort of related to what you said about the shadow. We could either experiment with the contrast or add another indicator of an active tab.

Yes, I wanted to solve these two things at once.

ravicious · 2023-05-30T14:53:22Z

web/packages/design/src/theme/lightTheme.ts

Not relevant to the PR, but on full HD the colors on smaller fonts seem to blur a little bit. This is less visible on the dark theme. Compare the Refresh button in the top right and the table headers.

I don't think it's necessarily the fault of the colors, those fonts could be just bigger.

Light Dark

I'm curious if this is related to: https://www.electronjs.org/docs/latest/faq#the-font-looks-blurry-what-is-this-and-what-can-i-do
I have not changed the browser window background color, so it still dark. Would like to test it when you have some time with light bg? (I don't have FHD display 😞)

I think the browser window background is already light since it uses theme.colors.levels.sunken. theme in this context is the light theme since I replaced it whole in the theme provider.

ravicious · 2023-05-30T15:01:39Z

I didn't even look at the code, I'll do so tomorrow.

…drop area as for the input

avatus

I didn't find anything else that @ravicious hadn't already pointed out. Looks fine to me

roraback · 2023-05-31T10:55:02Z

Not a blocker for merging, but for the tabs, the active tab should ideally merge with the rest of the window like they are part of the same page, sunken slightly below the other tabs. This may be tricky to pull off elegantly in HTML, but if nothing else we could fake it with some clever overlapping of elements to hide the shadow below the active tab and inset shadows to make it look like the tab has shadow falling on it, too.

ravicious

The code looks fine other than the one hardcoded color.

ravicious · 2023-05-31T12:50:12Z

web/packages/teleterm/src/ui/DocumentGateway/CliCommand.tsx

@@ -50,7 +50,8 @@ export function CliCommand({ cliCommand, onRun, isLoading }: CliCommandProps) {
    >
      <Flex
        mr="2"
-        color={shouldDisplayIsLoading ? 'text.slightlyMuted' : 'text.main'}
+        // always use light colors
+        color={shouldDisplayIsLoading ? 'rgba(255, 255, 255, 0.72)' : 'light'}


Could you remove the hardcoded color? It won't play nice with custom themes that we eventually want to add.

Not sure what are some of the ways we can solve this, perhaps we could manipulate the opacity instead?

I used the color modification function to manually add some transparency to the light color.

ravicious · 2023-05-31T14:53:12Z

msgcat --color=test output

stable dark theme	new dark theme	light theme

It looks like the new dark theme has rather row contrast between different text colors and the background colors compared to the current stable version.

gzdunek · 2023-05-31T15:45:03Z

The reason for that low contrast between different colors when colored background is used is the option minimumContrastRatio: 4.5 set in xterm. Without it, some of the combinations wouldn't be legible.
When you try that command in vscode you will get almost the same result, because it sets that option to 4.5 as well.

I'm not a heavy terminal user, but tbh I have not seen combinations like green on blue in a real use. Usually, when some background color is set, the foreground is black or white. But even if such combination is used, it seems to me that the readability of the text is more important than its color, right?

The only thing that I don't understand is black color in Colors (background) mixed with attributes: section (cell B2). Why the text doesn't have more contrast? It should have because of that option 🤔

ravicious · 2023-06-01T13:31:43Z

I see, in that case only the black and white backgrounds seem out of place. Is it maybe because xterm doesn't recognize the default color of the background properly? So when minimumContrastRatio kicks in, it compares the color against a wrong background, that's why the light theme uses gray for a white background.

Also, do you know why VSCode supports hue & saturation and we don't?

ravicious · 2023-06-01T13:40:14Z

Not a blocker for merging, but for the tabs, the active tab should ideally merge with the rest of the window like they are part of the same page, sunken slightly below the other tabs. This may be tricky to pull off elegantly in HTML, but if nothing else we could fake it with some clever overlapping of elements to hide the shadow below the active tab and inset shadows to make it look like the tab has shadow falling on it, too.

I suggested this back when we were adding that shadow to the top bar (#24883 (comment)) after Grzegorz pointed out the problem with the new colors (#24883 (comment)).

Grzegorz tried to make it work back then but it still required a few touch ups and IIRC we didn't have that much time back then to focus on the details.

gzdunek · 2023-06-01T15:44:01Z

Yeah, it looks like xterm incorrectly calculates final color when semitransparent background is used. I prepared a comparison of how different combinations look like together. By alpha colors I mean that black, brightBlack, white and brightWhite have an alpha channel. no-alpha colors were prepared by using lighten function:

brightWhite: lighten('#0C143D', 0.89),
white: lighten('#0C143D', 0.78),
brightBlack: lighten('#0C143D', 0.61),
black: lighten('#0C143D', 0.46),

alpha colors, no min contrast	alpha colors & min contrast 4.5	no-alpha colors & min contrast=4.5

Looks like the third options is the most legible one, I only miss the colors on a black background (cell C1) - they all look the same, it looks better in B1 :/

Also, do you know why VSCode supports hue & saturation and we don't?

Yes, I've just discovered it.
We have to change terminal name from name: 'xterm-color', to name: 'xterm-256color'. We can also set 'COLORTERM': 'truecolor' env var:

I think it requires testing on Windows, vscode doesn't use it there https://github.com/microsoft/vscode/blob/bfcb70bd09d53c99e9cfcf1e5072e8c165595628/src/vs/platform/terminal/node/terminalProcess.ts#L154

ravicious · 2023-06-02T11:28:11Z

I agree that the third column looks best. I think we could go with this for now and leave the hue & saturation stuff for later.

The colors on black background would probably look better if it was actually black and not grey but that's up to Kenny I guess. 😏

gzdunek · 2023-06-02T13:04:44Z

Right, the colors look good when black is #000.

A summary for @roraback:

We have to covert colors with an alpha channel to be non-transparent. It turned out that minimumContrastRatio sometimes doesn't work well with transparency. We have utility functions in code (lighten, darken) that can do it for us, so that's easy.
I will use different "base" color for WebUI and Connect as they have different terminal background in the dark theme. For example:

Connect:
brightBlack: lighten('#0C143D', 0.61)

WebUI:
brightBlack: lighten('#010B1C', 0.61)

Color "black" should be "black" as Rafał wrote (vscode also uses this value). But please remember, it is fully black only when used as a background (when used as a foreground, the color is modified by minimumContrastRatio):

roraback · 2023-06-02T21:12:10Z

Codifying Slack DM discussion: although Connect and the Web UI currently have different CLI background colors, I'd like these to be the same going forward; ideally Connect and the web UI will only differ when there's a functional reason for them to do so. In the design system in Figma, I set up all of the contrast ratios to work with the darkest blue background: #0c143d and the darkest off-white background, #e6e9ea. It is not a blocker for this PR, though—the web UI and Connect already have different background colors for the terminal theme, so it isn't a regression.

public-teleport-github-review-bot · 2023-06-05T13:20:29Z

@gzdunek See the table below for backport results.

Branch	Result
branch/v13	Create PR

* 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: 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 requested review from ravicious and avatus May 29, 2023 17:12

gzdunek added the backport/branch/v13 label May 29, 2023

github-actions bot requested a review from ryanclark May 29, 2023 17:12

github-actions bot added size/sm ui labels May 29, 2023

gzdunek removed the request for review from ryanclark May 29, 2023 17:13

gzdunek added 5 commits May 29, 2023 19:22

Adjust Connect to light theme

75e3fac

Remove clusters/* element

3a83bcc

Add terminal colors

0e8ff48

Remove warning about using false as color

db5edc2

Add custom styling for Toggle

a07d55c

gzdunek force-pushed the gzdunek/light-theme-adjustments branch from 1708ed5 to a07d55c Compare May 29, 2023 17:23

ravicious reviewed May 30, 2023

View reviewed changes

ravicious self-requested a review May 30, 2023 15:01

Fix light theme for file transfer, use the same border color for the …

23d9f42

…drop area as for the input

avatus approved these changes May 30, 2023

View reviewed changes

zmb3 approved these changes May 30, 2023

View reviewed changes

ravicious approved these changes May 31, 2023

View reviewed changes

Do not hardcode color in CliCommand

cc3441c

Use #000 as black

62c1782

Convert rgba colors to be non-opaque

3b16d4b

Fix two slightly incorrect colors

a6b2db7

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

Merged via the queue into master with commit ed2bb4b Jun 5, 2023
20 checks passed

gzdunek deleted the gzdunek/light-theme-adjustments branch June 5, 2023 13:19

zmb3 mentioned this pull request Jun 12, 2023

Incorporate CLI and YAML editor colors into light and dark themes #25413

Closed

gzdunek mentioned this pull request Jun 26, 2023

[v13] Connect: Light theme #28277

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Connect: Adjust to the light theme #27080

Connect: Adjust to the light theme #27080

gzdunek commented May 29, 2023 •

edited

ravicious left a comment

ravicious May 30, 2023

gzdunek May 30, 2023

ravicious May 30, 2023

gzdunek May 30, 2023

ravicious May 30, 2023

gzdunek May 30, 2023

ravicious May 31, 2023

ravicious commented May 30, 2023

avatus left a comment

roraback commented May 31, 2023

ravicious left a comment

ravicious May 31, 2023

gzdunek May 31, 2023

ravicious commented May 31, 2023

gzdunek commented May 31, 2023 •

edited

ravicious commented Jun 1, 2023

ravicious commented Jun 1, 2023

gzdunek commented Jun 1, 2023 •

edited

ravicious commented Jun 2, 2023

gzdunek commented Jun 2, 2023

roraback commented Jun 2, 2023 •

edited

public-teleport-github-review-bot bot commented Jun 5, 2023

Connect: Adjust to the light theme #27080

Connect: Adjust to the light theme #27080

Conversation

gzdunek commented May 29, 2023 • edited

ravicious left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravicious commented May 30, 2023

avatus left a comment

Choose a reason for hiding this comment

roraback commented May 31, 2023

ravicious left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravicious commented May 31, 2023

gzdunek commented May 31, 2023 • edited

ravicious commented Jun 1, 2023

ravicious commented Jun 1, 2023

gzdunek commented Jun 1, 2023 • edited

ravicious commented Jun 2, 2023

gzdunek commented Jun 2, 2023

roraback commented Jun 2, 2023 • edited

public-teleport-github-review-bot bot commented Jun 5, 2023

gzdunek commented May 29, 2023 •

edited

gzdunek commented May 31, 2023 •

edited

gzdunek commented Jun 1, 2023 •

edited

roraback commented Jun 2, 2023 •

edited