Theme refactor #358

KenanYusuf · 2021-05-06T07:56:55Z

This PR refactors the existing way of theming the dev tools.

Changes include:

Splitting the single theme into a lightTheme and darkTheme
Updated colour palette to one from Figma file
Using context-specific naming for each theme property
- The theory behind this is that it will encourage reuse of colours for similar bits of UI. For example, only two shades of grey will be used for text via theme.text.base and theme.textDimmed.base.
- Each colour property has a base property, as well any other relevant shades, tints and modifiers. For example, the primary property: primary: { base: colors.purple, hover: tint(0.1, colors.purple), ...others } see theme.ts for full example
Updates syntax highlighting to have a unique theme for both dark & light modes
- Based on prism-vs and prism-vsc-dark-plus with colours changed to accessible variants of updated colour palette
Added theme type support to styled components
Added ability to toggle theme in cosmos to help developing the tools with both themes in mind

All feedback on the approach and usage of colour welcome 🙏

Dark mode

Before	After

Light mode

Before	After

jmfulgham · 2021-05-06T17:25:36Z

src/panel/components/Background.tsx

@@ -3,7 +3,7 @@ import styled from "styled-components";
 export const Background = styled.div`
  overflow: hidden;
  position: fixed;
-  background-color: ${(props) => props.theme.dark[0]};
+  background-color: ${(p) => p.theme.canvas.base};


Should we keep this named props just for readability sake? Not that big of a deal but that's what came to mind when I saw this.

Yeah it's a good point! I noticed the majority of styled components used p instead of props so changed it for consistencies sake but I would also vouch for props or even destructuring the object ${({ theme }) => theme.canvas.base};

Just weighing in on this, I think all of the above seem totally fine to me!

I'm surprised at how much time I've saved by just writing p => p.theme instead of destructuring, it all adds up I guess 😅

jmfulgham

LGTM

ValGeorgiev

Thank you for doing this Kenan 🎉 Looks very good

ValGeorgiev · 2021-05-07T07:50:31Z

src/panel/cosmos.decorator.tsx

+  return (
+    <ThemeProvider {...props} theme={theme === "dark" ? darkTheme : lightTheme}>
+      <ThemeToggle
+        onClick={() => setTheme(theme === "dark" ? "light" : "dark")}


Nice, this is only for cosmos, isn't it? In the extension we are using chrome themes to detect if it's dark or white.

Yeah, exactly!

Potentially worth extracting this to a callback for some minor perf improvements

@wgolledge thanks for the suggestion! Updated here: 54582a5

wgolledge

Awesome stuff! 💯

wgolledge · 2021-05-07T09:13:28Z

src/panel/cosmos.decorator.tsx

+  return (
+    <ThemeProvider {...props} theme={theme === "dark" ? darkTheme : lightTheme}>
+      <ThemeToggle
+        onClick={() => setTheme(theme === "dark" ? "light" : "dark")}


Potentially worth extracting this to a callback for some minor perf improvements

wgolledge · 2021-05-07T09:15:35Z

src/panel/components/Background.tsx

@@ -3,7 +3,7 @@ import styled from "styled-components";
 export const Background = styled.div`
  overflow: hidden;
  position: fixed;
-  background-color: ${(props) => props.theme.dark[0]};
+  background-color: ${(p) => p.theme.canvas.base};


Just weighing in on this, I think all of the above seem totally fine to me!

I'm surprised at how much time I've saved by just writing p => p.theme instead of destructuring, it all adds up I guess 😅

wgolledge · 2021-05-07T09:18:26Z

src/panel/App.tsx

-  );
-};
+export const App: FC = () => (
+  <ThemeProvider theme={isLightMode() ? lightTheme : darkTheme}>


wgolledge · 2021-05-07T09:19:51Z

src/panel/theme.ts

@@ -1,344 +1,272 @@
-import { lighten, darken, mix, tint, shade } from "polished";
+import { tint, shade, mix } from "polished";


This whole theme breakdown seems so much more readable/semantic to me!

andyrichardson · 2021-05-10T09:14:38Z

src/panel/__snapshots__/App.test.tsx.snap

+      "syntax": Object {
+        "atom": "#e8e8e9",
+        "attrName": "#33b3ff",
+        "base": "#33b3ff",
+        "boolean": "#9291db",
+        "builtin": "#9291db",
+        "className": "#44b9cb",
+        "comment": "#6cc7a4",
+        "constant": "#33b3ff",
+        "description": "#a4a5a8",
+        "enum": "#6cc7a4",
+        "function": "#e8e8e9",
+        "input": "#9291db",
+        "interface": "#9291db",
+        "invalid": "#F65151",
+        "keyword": "#9291db",
+        "meta": "#F65151",
+        "null": "#a4a5a8",
+        "number": "#6cc7a4",
+        "operator": "#e8e8e9",
+        "property": "#33b3ff",
+        "punctuation": "#e8e8e9",
+        "scalar": "#ed9b3d",
+        "string": "#ed9b3d",
+        "type": "#33b3ff",
+        "union": "#44b9cb",
+        "variable": "#33b3ff",
      },


I like where you're going with this 👍

andyrichardson · 2021-05-10T09:17:47Z

src/panel/components/Toolbar.tsx


  &:hover {
-    color: ${(p) => (p.active ? p.theme.accent["+3"] : p.theme.light["-9"])};
+    color: ${(p) =>
+      p.active ? p.theme.primary.hover : p.theme.textDimmed.hover};


Loving the colocation of color and it's variants 🔥

andyrichardson

WOW 😍

Implementation looks spot on (imho) and the end result is 🔥 Accent color is spot on and the color contrast is a massive improvement!

Some thoughts:

Introduction of hue to the dark theme is a welcome change - I'm tempted to think we could keep the hue but make the canvas slightly darker so the increase the contrast with the primary color (massively subjective - I don't have a suggested color and you've likely experimented with this already so no need to make a change)
If you are interested, would be dope to add a global theme toggle to fixtures in a follow up PR see here

From the comments, it sounds like everyone is super stoked with these changes! Lets get them merged and HMU for the release process.

KenanYusuf · 2021-05-10T09:36:22Z

@andyrichardson thanks for the comments! Will follow up with a PR to add the global theme toggle, that seems like a nicer solution 😁

KenanYusuf added 6 commits May 5, 2021 15:31

Refactor dark and light theming across app

addd483

Refactor theme values into objects

21a47c9

Update visual regression snapshots

432b93f

Fix list item border colour

8b663de

Fix light/dark mode test

9b7a200

Update missing colours in CodeHighlight and Fields

4f85a83

KenanYusuf marked this pull request as ready for review May 6, 2021 10:08

jmfulgham reviewed May 6, 2021

View reviewed changes

jmfulgham approved these changes May 6, 2021

View reviewed changes

ValGeorgiev approved these changes May 7, 2021

View reviewed changes

wgolledge approved these changes May 7, 2021

View reviewed changes

Refactor cosmos theme toggle to use useCallback

54582a5

andyrichardson reviewed May 10, 2021

View reviewed changes

andyrichardson approved these changes May 10, 2021

View reviewed changes

KenanYusuf merged commit 5151119 into master May 10, 2021

KenanYusuf deleted the feature/theme-refactor branch May 10, 2021 09:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Theme refactor #358

Theme refactor #358

KenanYusuf commented May 6, 2021 •

edited

Loading

jmfulgham May 6, 2021 •

edited

Loading

KenanYusuf May 6, 2021

wgolledge May 7, 2021

jmfulgham left a comment

ValGeorgiev left a comment

ValGeorgiev May 7, 2021

KenanYusuf May 7, 2021

wgolledge May 7, 2021

KenanYusuf May 10, 2021

wgolledge left a comment

wgolledge May 7, 2021

wgolledge May 7, 2021

wgolledge May 7, 2021

wgolledge May 7, 2021

andyrichardson May 10, 2021

andyrichardson May 10, 2021

andyrichardson left a comment

KenanYusuf commented May 10, 2021

		@@ -1,344 +1,272 @@
		import { lighten, darken, mix, tint, shade } from "polished";
		import { tint, shade, mix } from "polished";

Theme refactor #358

Theme refactor #358

Conversation

KenanYusuf commented May 6, 2021 • edited Loading

Dark mode

Light mode

jmfulgham May 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmfulgham left a comment

Choose a reason for hiding this comment

ValGeorgiev 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

wgolledge 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

andyrichardson left a comment

Choose a reason for hiding this comment

KenanYusuf commented May 10, 2021

KenanYusuf commented May 6, 2021 •

edited

Loading

jmfulgham May 6, 2021 •

edited

Loading