Theme fixes by gzdunek · Pull Request #24883 · gravitational/teleport

gzdunek · 2023-04-20T12:47:41Z

This PR mainly adjusts Connect to the updated dark theme. Some of the changes affect also the WebUI, so @rudream I'd be grateful if you could take a look at the non-Connect commits (up to this one 80f2124).

Justification for the changes that affect WebUI:

~~1fe1d7b - the pagination icon is barely visible when disabled~~
f16209d - I don't know if there was any design for that, but here https://www.figma.com/file/Gpjs9vjhzUKF1GDbeG9JGE/Application-Design-System?node-id=7102%3A34980&t=r6OqTad9lQjTeVqw-1 Kenny used a different color. IMO looks better.
a27ec5e - on Firefox placeholders are a little more transparent https://developer.mozilla.org/en-US/docs/Web/CSS/::placeholder#opaque_text
f3db99d - I don't know if there was a design for it too, but secondary text seems to dark for list items (and doesn't meet accessibility requirements in Chrome dev tools).

@ravicious if something in Connect doesn't look right, feel free to commit fixes.

…nect custom tables

ravicious · 2023-04-20T13:20:58Z

  padding: ${props => props.theme.space[2]}px;
  color: ${props => props.theme.colors.text.contrast};
-  background: ${props => props.theme.colors.levels.surface};
+  background: inherit;


This can be removed.

ravicious · 2023-04-20T13:31:48Z

-        &:active {
-          border: 1px solid ${theme.colors.text.secondary};
-        }
+            background: inherit;


This is to work around the fact that form elements are not aware of the surface level they're used at, right?

@rudream did you run into similar problems in the Web UI? This feels like an interesting problem to take care of in the future.

This is the background of the textarea that is shown when you click on the little chat icon in the bottom right corner.

Well, maybe it's not even that interesting. If we want those field forms to be flat, they could just not set the background color by themselves and instead inherit it, as shown here.

I think it makes sense to inherit parent color, but I was afraid to change it for Input which is used in a lot of places :/

@rudream did you run into similar problems in the Web UI? This feels like an interesting problem to take care of in the future.

For the inputs I generally used transparent background as opposed to inherit (I think in this case the end result is the same). Yes, this is because the element isn't aware of what surface level it's at, using a solid colour background would look like transparency on a background of the same colour but would look out of place on a different elevation (such as in a dialog).

ravicious · 2023-04-20T13:35:13Z

  },

-  brand: '#512FC9',
+  brand: '#9F85FF',


Shouldn't we import those directly from the dark theme instead of copying them here? In the end, shouldn't the Web UI and Connect use the same themes? cc @rudream

I noticed that even before the theme updates, Connect used slightly different colours than the Web UI (you can compare the theme.ts files for both in branch/v12). I didn't want to disrupt this as I know we don't have official designs for the updated Connect themes yet so I duplicated some colours just for the sake of compatibility with our shared design components.

I don't know if the Connect dark theme is supposed to look exactly like the Web UI dark theme (since it didn't in the past), but if it does then yes we should be importing the themes from the Web UI as opposed to using a duplicate. I did this to get the MVP for the Web UI out in time without breaking Connect since that was the priority.

ravicious · 2023-04-20T13:38:13Z

I'll try out those changes tomorrow, so far I've been just going through the code and used the search bar.

This reverts commit 1fe1d7b.

ravicious

Love to see Connect just directly importing the colors from the dark theme. Great job @gzdunek and @rudream!

ravicious · 2023-04-26T14:33:49Z

+        &:hover > input {
+          background: ${props => props.theme.colors.spotBackground[0]};


This could be defined on Input itself, couldn't it?

# Conflicts: # e # web/packages/design/src/Input/Input.jsx # web/packages/design/src/Menu/MenuItem.jsx # web/packages/shared/components/FieldInput/__snapshots__/FieldInput.test.tsx.snap # web/packages/shared/components/FormPassword/__snapshots__/FormPassword.test.tsx.snap # web/packages/shared/components/MenuLogin/MenuLogin.tsx # web/packages/teleport/src/Account/ManageDevices/AddDevice/__snapshots__/AddDevice.story.test.tsx.snap # web/packages/teleport/src/Welcome/NewCredentials/__snapshots__/NewCredentials.story.test.tsx.snap # web/packages/teleport/src/components/FormLogin/__snapshots__/FormLogin.story.test.tsx.snap # web/packages/teleterm/src/ui/DocumentCluster/ClusterResources/ClusterSearch.tsx # web/packages/teleterm/src/ui/DocumentCluster/ClusterResources/MenuLoginTheme.tsx # web/packages/teleterm/src/ui/QuickInput/QuickInput.tsx # web/packages/teleterm/src/ui/QuickInput/QuickInputList/QuickInputList.tsx # web/packages/teleterm/src/ui/Search/SearchBar.tsx # web/packages/teleterm/src/ui/Search/pickers/PickerContainer.ts # web/packages/teleterm/src/ui/Search/pickers/ResultList.tsx # web/packages/teleterm/src/ui/StatusBar/ShareFeedback/ShareFeedbackFormFields.tsx # web/packages/teleterm/src/ui/Tabs/TabItem.tsx # web/packages/teleterm/src/ui/ThemeProvider/theme.ts # web/packages/teleterm/src/ui/TopBar/Clusters/ClustersFilterableList/ClusterItem.tsx # web/packages/teleterm/src/ui/components/FilterableList/FilterableList.tsx # web/packages/teleterm/src/ui/components/ListItem.tsx # web/packages/teleterm/src/ui/components/Table.tsx

ravicious

Left some comments about some minor issues but the rest looks good!

ravicious · 2023-04-27T15:41:46Z

      {!isOpen && (
        <>
-          <Input {...defaultInputProps} />
+          <Input {...defaultInputProps} css={`


By this being defined on the input itself I meant moving it within the component declaration that starts with const Input = styled.input.

Though what's wrong with defining the background color the way it was, on the flex itself?

teleport/web/packages/teleterm/src/ui/Search/SearchBar.tsx

Lines 114 to 117 in abcd527

&:hover {

color: ${props => props.theme.colors.levels.contrast};

background: ${props => props.theme.colors.levels.surface};

}

When I added the semi-transparent hover on the div level, it was brighter than it should (like alpha was 0.14 not 0.07). I don't know why this happened, the input inherited background from the parent. For some reason hover on the div and the input looked a bit different.
But now I found a way to fix it, I only need to set transparent background for the input.

ravicious · 2023-04-27T15:44:12Z

      visible={visible}
      flexDirection="column"
      pl={2}
+      pt={1}


Looks good but let's add a comment explaining what the top padding is for.

ravicious · 2023-04-27T15:51:02Z

          max-width: ${maxWidth};
          min-width: 0;
          flex: 1;
+          background-color: ${props => props.theme.colors.levels.elevated};


Let's replace <Flex gap={4}> above with <Flex gap={4} alignItems="flex-start"> to get rid of the second column stretching all the way down.

rudream

LGTM

* Use a better icon for the reverse tunnel cell * Use `ButtonIcon` for pagination icons * Fix rendering table border on Safari * Manually add a visual separator between the table and the element below it * Add bottom padding to resource tables in Connect * Update snapshots

# Conflicts: # e # web/packages/teleport/src/Audit/__snapshots__/Audit.story.test.tsx.snap

public-teleport-github-review-bot · 2023-05-04T15:34:32Z

@gzdunek See the table below for backport results.

Branch	Result
branch/v13	Failed

* Theme fixes (#24883) * Make pagination icons a little more visible * Use different color for `tr` border * Use `opacity: 1` for placeholders * Include `borderRadius` in `StyledPanel` to fix rounded corners in Connect custom tables * Use `colors.text.primary` for items in `MenuLogin` * Adjust Connect theme to the updated dark theme * Remove unused component * Update snapshots * Remove `surfaceSecondary` and `sunkenSecondary` colors * Remove unneeded `inherit` * Do not hardcode bg color in `TextArea` * Expand comment * Simplify the look of top bar elements * Remove unused component * Remove `text.contrast` * Use the same dark theme for WebUI and Connect * Revert "Make pagination icons a little more visible" This reverts commit 1fe1d7b. * Add shadow for the tabs * Post-merge fixes * Do not use Arial on custom buttons * Revert snapshot changes * Fix colors in `ActionPicker` * Apply hover styles directly on `SearchBar` input * Use white color for "Database Connection" header * Add shadow directly to `StyledTabs` * Run prettier * Update e * Add some top padding to the terminal * Review fixes * Move SearchBar hover to the Flex element * Table-related improvements (#25333) * Use a better icon for the reverse tunnel cell * Use `ButtonIcon` for pagination icons * Fix rendering table border on Safari * Manually add a visual separator between the table and the element below it * Add bottom padding to resource tables in Connect * Update snapshots * Update snapshots * Revert e (cherry picked from commit 1870c80) * Update `Session.story.test.tsx.snap`

gzdunek added 2 commits April 20, 2023 14:19

Make pagination icons a little more visible

1fe1d7b

Use different color for tr border

f16209d

gzdunek added the backport/branch/v13 label Apr 20, 2023

gzdunek requested review from ravicious and rudream April 20, 2023 12:47

github-actions Bot added size/sm ui labels Apr 20, 2023

github-actions Bot requested a review from ryanclark April 20, 2023 12:48

gzdunek removed the request for review from ryanclark April 20, 2023 12:48

gzdunek added 6 commits April 20, 2023 14:50

Use opacity: 1 for placeholders

a27ec5e

Include borderRadius in StyledPanel to fix rounded corners in Con…

9773f75

…nect custom tables

Use colors.text.primary for items in MenuLogin

f3db99d

Adjust Connect theme to the updated dark theme

6abd479

Remove unused component

868d554

Update snapshots

b918458

gzdunek force-pushed the gzdunek/theme-updates branch from 338b880 to b918458 Compare April 20, 2023 12:58

ravicious reviewed Apr 20, 2023

View reviewed changes

ravicious self-requested a review April 20, 2023 13:38

roraback reviewed Apr 20, 2023

View reviewed changes

Comment thread web/packages/design/src/DataTable/Pager/StyledPager.tsx Outdated

Comment thread web/packages/teleterm/src/ui/Search/pickers/ResultList.tsx

gzdunek added 10 commits April 26, 2023 11:28

Remove surfaceSecondary and sunkenSecondary colors

c9ae1c8

Remove unneeded inherit

f703a27

Do not hardcode bg color in TextArea

4e5d828

Expand comment

8d876a1

Simplify the look of top bar elements

cd659c3

Remove unused component

a6381ed

Remove text.contrast

7ca6fd9

Use the same dark theme for WebUI and Connect

5c8b744

Revert "Make pagination icons a little more visible"

b6cff55

This reverts commit 1fe1d7b.

Add shadow for the tabs

8c1b5c9

ravicious reviewed Apr 26, 2023

View reviewed changes

Comment thread web/packages/teleterm/src/ui/Tabs/Tabs.tsx Outdated

ravicious reviewed Apr 26, 2023

View reviewed changes

gzdunek added 11 commits April 27, 2023 09:28

Post-merge fixes

db0d3f8

Do not use Arial on custom buttons

24eb739

Revert snapshot changes

c49b1d9

Fix colors in ActionPicker

fdccc4c

Apply hover styles directly on SearchBar input

c1a1bd0

Use white color for "Database Connection" header

754967c

Add shadow directly to StyledTabs

c662518

Run prettier

0828738

Update e

f32d750

Add some top padding to the terminal

8b88ff0

ravicious approved these changes Apr 27, 2023

View reviewed changes

gzdunek added 2 commits April 27, 2023 18:11

Review fixes

bc7c88c

Move SearchBar hover to the Flex element

d7d8762

rudream approved these changes May 4, 2023

View reviewed changes

gzdunek and others added 4 commits May 4, 2023 16:30

Merge branch 'master' into gzdunek/theme-updates

bc64508

# Conflicts: # e # web/packages/teleport/src/Audit/__snapshots__/Audit.story.test.tsx.snap

Update snapshots

0c607cc

Revert e

a53eb15

gzdunek added this pull request to the merge queue May 4, 2023

Merged via the queue into master with commit 1870c80 May 4, 2023

gzdunek deleted the gzdunek/theme-updates branch May 4, 2023 15:33

gzdunek mentioned this pull request May 4, 2023

[v13] Theme fixes (#24883) #25640

Merged

ravicious mentioned this pull request Jun 1, 2023

Connect: Adjust to the light theme #27080

Merged

r0mant mentioned this pull request Jul 14, 2023

Release 13.2.2 #29158

Closed

		&:hover > input {
		background: ${props => props.theme.colors.spotBackground[0]};

	&:hover {
	color: ${props => props.theme.colors.levels.contrast};
	background: ${props => props.theme.colors.levels.surface};
	}

Conversation

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ravicious commented Apr 20, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ravicious left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

rudream left a comment

Choose a reason for hiding this comment

Uh oh!

public-teleport-github-review-bot Bot commented May 4, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gzdunek commented Apr 20, 2023 •

edited

Loading