RFC: Theming in the future #973

emmatown · 2018-10-28T02:12:32Z

I wrote my thoughts on what I think theming should look like with Hooks as documentation so read it on GitHub or on the deploy preview.

I think this is what we should primarily recommend because I’m not sure about the practicality of #887 in the real world and I’m not sure if the trade off in terms of dynamicism will really translate to an important performance improvement in the real world. I'm thinking of publishing it outside of the emotion packages and maybe trying it in a real project to see how well it works and then think about making it an official emotion package.

We should also absolutely keep on supporting the current theming API for at the absolute minimum, v10 but not provide the theme to the css prop, Global or ClassNames.

Andarist · 2018-10-28T08:07:10Z

docs/theming.md

+
+### Global Namespace
+
+Since there’s was a single object which contained the theme, if a component from npm or somewhere wanted to use theming, it had to choose a unique name store it’s theme in which meant name conflicts could occur. By directly using the Context API, this problem doesn't exist.


This shouldnt be relevant point here because it refers to legacy context and it's not a problem with new context APi.

It's relevant to the current theming solution. There's only a single object in the current theming API which means libraries have to namespace.

oh, i have kinda skipped the part where you mention libraries built on top of emotion 😅

Andarist · 2018-10-28T08:09:01Z

docs/theming.md

+
+### Testing
+
+To test components that used the previous theming API, either a default theme has to be provided with defaultProps or a ThemeProvider has to be wrapped around every component which could get really inconvenient. By providing a default value when the theme is created, components can just be rendered.


I don't feel strongly about this one - you might improve DX for testing here, but you deteriorate (to some extent) DX for writing components

How does it deteriorate DX for writing components?

I agree that this isn't the strongest point though, it was part of a point about people using theming when it's not really necessary but it didn't make a lot of sense so I changed it to this. It's kind of more about having to provide a default value so the component can be rendered anywhere without requiring a certain ancestor.

How does it deteriorate DX for writing components?

Current way of using theme is way more ergonomic (in my opinion) than having to manually use theme context

Andarist · 2018-10-28T08:09:52Z

docs/theming.md

+
+To test components that used the previous theming API, either a default theme has to be provided with defaultProps or a ThemeProvider has to be wrapped around every component which could get really inconvenient. By providing a default value when the theme is created, components can just be rendered.
+
+### Over-subscription


Preferring css variables for theming this also is not quite a problem, right?

Oops, I didn't finish this bit. It's meant to talk about every styled component having to subscribe to the theme even if it doesn't use it.

Andarist · 2018-10-28T08:11:06Z

docs/theming.md

 /** @jsx jsx */
 import { jsx } from '@emotion/core'
+import { useTheme } from './theme'
+
+let Button = props => {


this recommendation suffers from shouldForwardProp problem, which really gets annoying long term from DX perspective

Maybe this example shouldnt mention button but rather a composite component

Why? Use rest spread if you have a prop you want to use that shouldn't be passed down.

IMO, it's much better than styled in this regard because there's a native language feature for stopping certain props from being forwarded rather than having to learn a library-specific API.

right, ive misjudged the example by not thinking about it too much and having styled API with interpolations in mind, css prop doesnt suffer from this

We should probably dedicate a page on the docs dedicated to mapping solutions to "known" apis like withComponent, shouldForwardProp, etc. We should also show how easy it is to add further functionality to components this way. We could link to that page from here.

tkh44

This is great. Great work on these docs.

tkh44 · 2018-10-29T15:47:05Z

docs/theming.md

+### Global Namespace
+
+Since there was a single object which contained the theme, if a component from npm or somewhere wanted to use theming, it had to choose a unique name store it’s theme in which meant name conflicts could occur. By directly using the Context API, this problem doesn't exist.
+


An example here would emphasize this point because even though it is a great one, it can be hard to visualize.

tkh44 · 2018-10-29T16:19:13Z

docs/theming.md

 /** @jsx jsx */
 import { jsx } from '@emotion/core'
+import { useTheme } from './theme'
+
+let Button = props => {


We should probably dedicate a page on the docs dedicated to mapping solutions to "known" apis like withComponent, shouldForwardProp, etc. We should also show how easy it is to add further functionality to components this way. We could link to that page from here.

ianstormtaylor · 2019-02-13T20:17:50Z

I totally agree with this, this is a great direction. useTheme will be the easiest way to mentally think about these things.

My only request would be that emotion ships with the following as default:

import { createContext, useContext } from 'react'
const ThemeContext = createContext({})
export const ThemeProvider = ThemeContext.Provider
export const useTheme = () => useContext(ThemeContext)

That way for the simple cases we can just import { ThemeProvider, useTheme } from emotion directly without having to define the context ourselves. And then people are free to define their own theme context if they really need to, with the default values.

Otherwise it just adds to the boilerplate.

Right?

changeset-bot · 2019-11-13T06:57:48Z

💥 No Changeset

Latest commit: d8a5f4a

Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂

If these changes should be published to npm, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

codesandbox-ci · 2019-11-13T06:59:20Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit d8a5f4a:

Sandbox	Source
Emotion	Configuration

stramel · 2019-12-20T17:54:59Z

docs/theming.mdx

-  colors: {
-    primary: 'hotpink'
+let Button = styled.button(() => {
+  let theme = useTheme()


This usage and the below usage trigger errors in the latest React.

Uncaught Error: Invalid hook call. Hooks can only be called inside of the body of a function component. This could happen for one of the following reasons: 1. You might have mismatching versions of React and the renderer (such as React DOM) 2. You might be breaking the Rules of Hooks 3. You might have more than one copy of React in the same app See https://fb.me/react-invalid-hook-call for tips about how to debug and fix this problem.

This is the line it errors on

const Card = styled.div` background-color: ${() => useTheme().palette.white}; /* Error */ `

@stramel have you tried to use this with emotion 10 or 11? It can only work with the latter.

Oh let me try! Thank you for the quick reply!

@Andarist I'm running it inside Docz v2 using @emotion/styled and @emotion/core on version 11.0.0-next.9.

When starting the project it complains because it can't find @emotion/styled-base.

ERROR #98123 WEBPACK Generating SSR bundle failed Can't resolve '@emotion/styled-base' in 'Projects/libs/atoms/src/lib/Button'

so I installed @emotion/styled-base version v11.0.0-next.3 and it is telling me i need to use @emotion/styled?

When I tried the v10 version of @emotion/styled-base it obviously failed... 😞

Seems like maybe you have 2 versioms of emotion in your node modules for some reason or that you are using v10 of our babel plugin. @emotion/styled-base has been removed in v11 in favor of @emotion/styled/base.

If you prepare a runnable repro case then I could take a look at it.

@Andarist Looks like you were correct. Just tried it in my Next.js app with v11 and no errors. Doing some debugging...

yarn why @emotion/core and yarn why @emotion/styled both result in v10 and v11 because Docz and the gatsby docz theme both use v10

Any suggestions how to fix it?

I filed an issue on docz asking for a bumping to v11 but I'm not sure what the workaround would be. doczjs/docz#1335

Thank you @stramel! Anyone know the expected release date of v11?

I can't promise anything, but hopefully within a month. We have most of the things done, some final touches left, but this is a work we are doing in our free time so there is also a possibility that this will take some more time.

JakeGinnivan · 2019-12-30T07:03:14Z

@mitchellhamilton I think the work on theming in v11 is better than this approach of stripping it out?

Thoughts on closing this?

Andarist · 2019-12-30T08:20:18Z

We dont intend to remove builtin theming but some of the problems with it were nicely outlined here. We should „compile” those parts into docs and recommend using builtin theming only when building apps while using custom theme approach where developing libraries on top of emotion

Theming in v10

c27a172

Andarist reviewed Oct 28, 2018

View reviewed changes

Update theming.md

c9fff45

emmatown mentioned this pull request Oct 29, 2018

Remove theme from new APIs #974

Closed

3 tasks

tkh44 approved these changes Oct 29, 2018

View reviewed changes

deep-c mentioned this pull request Nov 2, 2018

v10 theming and react hooks? #983

Closed

emmatown mentioned this pull request Nov 4, 2018

Primitive components with the CSS Prop doc #992

Closed

3 tasks

emmatown changed the title ~~RFC: Theming in v10 and beyond~~ RFC: Theming in the future Nov 12, 2018

emmatown mentioned this pull request Nov 28, 2018

New theming API #887

Closed

3 tasks

This was referenced Nov 3, 2019

Namespacing themes #1241

Closed

Default theme in HOC withTheme #1193

Closed

emmatown added this to the v11 milestone Nov 8, 2019

Andarist mentioned this pull request Nov 12, 2019

Quick pass at removing theme generic param tomsseisums/emotion#1

Merged

emmatown changed the base branch from master to next November 13, 2019 06:56

Merge branch 'next' into theming-in-v10

d8a5f4a

github-actions bot mentioned this pull request Nov 13, 2019

Version Packages (next) #1625

Merged

Andarist mentioned this pull request Nov 23, 2019

Updated types to support global Theme definition #1609

Merged

4 tasks

anicholls mentioned this pull request Nov 26, 2019

feat(labs): Theming (react) Workday/canvas-kit#272

Merged

9 tasks

stramel reviewed Dec 20, 2019

View reviewed changes

anicholls mentioned this pull request Apr 21, 2020

Improve theming API stability Workday/canvas-kit#591

Closed

Andarist removed this from the v11 milestone Sep 8, 2020

Base automatically changed from next to master November 12, 2020 00:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: Theming in the future #973

RFC: Theming in the future #973

emmatown commented Oct 28, 2018 •

edited

Loading

Andarist Oct 28, 2018

emmatown Oct 28, 2018

Andarist Oct 28, 2018

Andarist Oct 28, 2018

emmatown Oct 28, 2018 •

edited

Loading

Andarist Oct 28, 2018

Andarist Oct 28, 2018

emmatown Oct 28, 2018

Andarist Oct 28, 2018

Andarist Oct 28, 2018

emmatown Oct 28, 2018

emmatown Oct 28, 2018

Andarist Oct 28, 2018

tkh44 Oct 29, 2018 •

edited

Loading

tkh44 left a comment

tkh44 Oct 29, 2018

tkh44 Oct 29, 2018 •

edited

Loading

ianstormtaylor commented Feb 13, 2019 •

edited

Loading

changeset-bot bot commented Nov 13, 2019

codesandbox-ci bot commented Nov 13, 2019

stramel Dec 20, 2019 •

edited

Loading

Andarist Dec 20, 2019

stramel Dec 21, 2019

stramel Dec 21, 2019

Andarist Dec 21, 2019

stramel Dec 21, 2019

weyert Dec 23, 2019

stramel Dec 23, 2019

weyert Dec 23, 2019

Andarist Dec 23, 2019

JakeGinnivan commented Dec 30, 2019

Andarist commented Dec 30, 2019


		### Global Namespace

		Since there’s was a single object which contained the theme, if a component from npm or somewhere wanted to use theming, it had to choose a unique name store it’s theme in which meant name conflicts could occur. By directly using the Context API, this problem doesn't exist.


		### Testing

		To test components that used the previous theming API, either a default theme has to be provided with defaultProps or a ThemeProvider has to be wrapped around every component which could get really inconvenient. By providing a default value when the theme is created, components can just be rendered.


		To test components that used the previous theming API, either a default theme has to be provided with defaultProps or a ThemeProvider has to be wrapped around every component which could get really inconvenient. By providing a default value when the theme is created, components can just be rendered.

		### Over-subscription

		### Global Namespace

		Since there was a single object which contained the theme, if a component from npm or somewhere wanted to use theming, it had to choose a unique name store it’s theme in which meant name conflicts could occur. By directly using the Context API, this problem doesn't exist.

RFC: Theming in the future #973

Are you sure you want to change the base?

RFC: Theming in the future #973

Conversation

emmatown commented Oct 28, 2018 • 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

emmatown Oct 28, 2018 • 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

tkh44 Oct 29, 2018 • edited Loading

Choose a reason for hiding this comment

tkh44 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkh44 Oct 29, 2018 • edited Loading

Choose a reason for hiding this comment

ianstormtaylor commented Feb 13, 2019 • edited Loading

changeset-bot bot commented Nov 13, 2019

💥 No Changeset

codesandbox-ci bot commented Nov 13, 2019

stramel Dec 20, 2019 • 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

JakeGinnivan commented Dec 30, 2019

Andarist commented Dec 30, 2019

emmatown commented Oct 28, 2018 •

edited

Loading

emmatown Oct 28, 2018 •

edited

Loading

tkh44 Oct 29, 2018 •

edited

Loading

tkh44 Oct 29, 2018 •

edited

Loading

ianstormtaylor commented Feb 13, 2019 •

edited

Loading

stramel Dec 20, 2019 •

edited

Loading