Theme provider #33

ciaranschutte · 2022-03-21T16:38:33Z

Description of changes

NB: most of these files are just updating our theme.foo references to theme.uikit.foo

Lots of Theme editing

sets up Jest + React Testing Library + Babel correctly to run tests and not break build. don't break build.
adds a Template component for testing but also as a... well.. component template
removed indexing. there's no 'i' in theme. (just a joke comment to ensure we're all reading every line)
namespaces the uikit theme because it will likely be used with other theme providers (helps to avoid variable collisions)
updates to readme

Type of Change

Bug
Styling
New Feature

Checklist before requesting review:

justincorrigible

Very namespace. Much theme 🐕

Couple questions, for me to understand some specific decisions.

justincorrigible · 2022-03-22T19:21:20Z

README.md

+
+Themes will merge with same versions of emotion
+UIKit theme is namespaced
+Please use as last theme provider


"last" may require a bit more elaboration... does this mean the provider needs to be nested as a child of all other (external) theme providers?
e.g. Arranger theme provider. Curious about the results for that overlap, if it was to ever happen 🤔

Yes 'last' is rather vague. To be honest it doesn't matter too much.
Once the same emotion version is in use it will be merged anyway, thus the namespacing.
so:

<ArrangerTProvider theme={{colors: blue}> <UIKitTProvider theme={{colors:blue}}> .....

theme would look something like:

{ colors: blue; uikit: {colors:blue} }

that makes all the sense in the world.

followup question: if your provider inherits { colors: blue, uikit: null } from a parent provider?
and if that overwrites the default theme, how do the components respond? e.g. theme.uikit.colors.blue?

justincorrigible · 2022-03-22T19:23:39Z

jest-setup.ts

+
+expect.extend(matchers);
+
+export {};


I figure this is boilerplate standard... but out of curiosity, what's it function?
may be useful to add a comment (e.g. within the curlies) to explain something so eager beavers like myself don't go deleting that line thinking it's cruft.

Good idea. It's just to make TS happy so it's a valid module seen as we're not exporting anything, just setting up some stuff

justincorrigible · 2022-03-22T19:26:54Z

package.json

-    "react": "16.x.x || 17.x.x",
-    "react-dom": "16.x.x || 17.x.x"


I may be wrong, but react and react-dom may still need to be peer deps, so that React doesn't throw mismatching versions for hooks.
That's happened to me while using npm/yarn link.

I think you're right. This has proven to be a big issue so I'm tackling this in a seperate PR.

justincorrigible · 2022-03-22T19:27:29Z

package.json

    "ts-node-dev": "^1.1.8",
    "tsc-watch": "^4.5.0",
-    "typescript": "^3.9.5"
+    "typescript": "^4.6.2"


justincorrigible · 2022-03-22T19:28:17Z

src/AppBar/DropdownMenu.tsx

@@ -23,16 +23,16 @@ import styled from '@emotion/styled';
 import clsx from 'clsx';

 const Ul = styled('ul')<{ theme?: any }>`
-  ${({ theme }) => css(theme.typography.paragraph)};
+  ${({ theme }) => css(theme.uikit.typography.paragraph)};


does the theme system allow for the theme to be disabled -> missing, or overwritten with a custom theme that has an entirely different shape?
in that case these chains may fail with cannot find X of undefined.

theme?.uikit?.typography?.paragraph covers that, but could also use having a fallback/default value if it the chain ever fails.

I like the idea of a fallback. This is an issue we currently face in our projects so it's not an introduced issue but could be a lovely improvement!

A missing theme will default to a default theme.

As for a different shape TS will yell at you but it could happen. I like having a very defined interface for anything we are passing to uikit. There's a lot of improvement to be made on this. plenty of places where we say we're using a theme color but then actually just import a color, so it's never actually dynamically theme-able to the outside. I will create a ticket with this.

justincorrigible · 2022-03-22T19:33:21Z

src/ThemeProvider/ThemeProvider.test.tsx

+
+const ThemesWrapper = ({ projectTheme = {}, uikitTheme = undefined, children }) => (
+  <ProjectThemeProvider theme={projectTheme}>
+    {/*@ts-ignore*/}


sheer curiosity: what checks is this avoiding?

I think this is something I ended up fixing :)
will double check and update. TS didn't like the shape of my theme prop

🤔 yeah, the TS inference vs annotation gets finicky, doesn't it?

justincorrigible · 2022-03-22T19:36:16Z

src/ThemeProvider/index.tsx

@@ -44,4 +36,4 @@ const ThemeProvider: React.ComponentType<{ theme?: keyof typeof themes }> = ({
 };

 export default ThemeProvider;
-export const useTheme = () => React.useContext(ThemeContext as React.Context<typeof defaultTheme>);
+export { useTheme } from '@emotion/react';


0% a criticism or complaint (as I get why this export is happening)... and yet, honestly, this pattern makes me uneasy.

would love to discuss more :) are you thinking use a regular context we create?

Not really. I did take that route in Arranger to prevent headches, but I'm not advocating for that approach.

It's just the exporting a third party module what doesn't seem right to me, but I may be being too picky.
Again, I know why it's needed, given Emotion's "special needs". Just thinking there may be other ways to have Emotion play along without this railguard enforcement.

It could be cleaned up. I think it'll work just fine if useTheme is directly imported from emotion where ever it is needed.
This re-export was mostly a "don't break more than you need to" change.

fair enough!!! "taking one day at a time" 👍 I like it.

justincorrigible · 2022-03-22T19:37:26Z

tsconfig.json

@@ -12,7 +12,8 @@
    "moduleResolution": "node",
    "resolveJsonModule": false,
    "isolatedModules": false,
-    "jsx": "preserve",
+    "jsx": "react-jsx",
+    "jsxImportSource": "@emotion/react",


nice! React 17 runtime! 🚀

this was a "fun" one... a lot of heartache between babel and jest until finally stumbling on jsxImportSource which has the added boon of not needing the pragma to be added everywhere.

yep. stumbled on this one myself a while ago. happy you were able to make it work!

justincorrigible

LGTM! 🚢

ciaranschutte added 8 commits January 31, 2022 00:55

inline svg for dist, remove need for require loader for parent project

b563c0b

setup test. remove old test commands

9bca7c3

emotion types

7e8a5eb

readme update

da30931

add libs

c98849b

add component template

2aa9c3d

export theme as object

5f72517

fix button import

0eb24c0

ciaranschutte changed the base branch from develop to v2 March 21, 2022 18:29

ciaranschutte added 2 commits March 21, 2022 15:23

ThemeProvider should take a theme object not a theme key

e9d4b69

theme provider tests

4cdba56

ciaranschutte marked this pull request as ready for review March 21, 2022 19:26

ciaranschutte added 11 commits March 22, 2022 00:41

update theme tests

953ea09

fix jest setup

c56e7b7

fix emotion typings

b635a54

use CSS api

456963b

use emotion 11 apis, cleanup theme provider

75081c0

update readme and package lock

0a384e8

update theme usage

896c129

adjust babel config style

a37bc03

fix babel and typescript pragma fighting

c7af42b

make theme optional, there is a default

986336b

Merge branch 'v2' into theme-provider

b0d0208

ciaranschutte requested review from justincorrigible, joneubank, anncatton, demariadaniel and mistryrn March 22, 2022 19:13

Update babel.config.js

3310eb4

justincorrigible reviewed Mar 22, 2022

View reviewed changes

This was referenced Mar 22, 2022

Theme property checking #34

Open

Setup correct peer dependencies #35

Open

ciaranschutte added 2 commits March 22, 2022 16:49

remove redundant ts-ignore

a21356a

remove redundant imports

da0bfb1

ciaranschutte requested a review from justincorrigible March 22, 2022 21:06

justincorrigible approved these changes Mar 22, 2022

View reviewed changes

ciaranschutte merged commit 4f55c88 into v2 Mar 22, 2022

ciaranschutte deleted the theme-provider branch March 22, 2022 21:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Theme provider #33

Theme provider #33

ciaranschutte commented Mar 21, 2022 •

edited

Loading

justincorrigible left a comment

justincorrigible Mar 22, 2022

ciaranschutte Mar 22, 2022 •

edited

Loading

justincorrigible Mar 22, 2022 •

edited

Loading

justincorrigible Mar 22, 2022

ciaranschutte Mar 22, 2022

justincorrigible Mar 22, 2022

ciaranschutte Mar 22, 2022

justincorrigible Mar 22, 2022

justincorrigible Mar 22, 2022

ciaranschutte Mar 22, 2022

justincorrigible Mar 22, 2022

ciaranschutte Mar 22, 2022

justincorrigible Mar 22, 2022

justincorrigible Mar 22, 2022

ciaranschutte Mar 22, 2022

justincorrigible Mar 22, 2022 •

edited

Loading

ciaranschutte Mar 22, 2022

justincorrigible Mar 22, 2022

justincorrigible Mar 22, 2022

ciaranschutte Mar 22, 2022

justincorrigible Mar 22, 2022

justincorrigible left a comment

Theme provider #33

Theme provider #33

Conversation

ciaranschutte commented Mar 21, 2022 • edited Loading

justincorrigible left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ciaranschutte Mar 22, 2022 • edited Loading

Choose a reason for hiding this comment

justincorrigible Mar 22, 2022 • edited Loading

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

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

justincorrigible Mar 22, 2022 • edited Loading

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

justincorrigible left a comment

Choose a reason for hiding this comment

ciaranschutte commented Mar 21, 2022 •

edited

Loading

ciaranschutte Mar 22, 2022 •

edited

Loading

justincorrigible Mar 22, 2022 •

edited

Loading

justincorrigible Mar 22, 2022 •

edited

Loading