Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor color objects and expose default colors as a mode #1639

Merged
merged 16 commits into from
Apr 28, 2021

Conversation

fcisio
Copy link
Collaborator

@fcisio fcisio commented Apr 12, 2021

Howdy 👋
This PR aims to expose the color modes to the context, in an immutable object.

Currently, all color objects are mutated upon color mode change. Which is not optimal when needing to access the "default" colors.

I took the liberty of changing the shape of the object, to allow for easier handling of all the modes.

theme: {
  allColorModes: {
    // __default: {...} -> if initialColorModeName is undefined
    light: {...},
    dark: {...},
  }
}

This object could be used in many ways, but here is a simple example:

Map modes and outputs themed buttons

Object.entries(allColorModes).map(([mode, values]) => ({
  <Button
    sx={{ bg: values.background, color: values.text }}
    onClick={() => setColorMode(mode)}
  >
    {mode}
  </Button>
}))

Version

Published prerelease version: v0.8.0-develop.0

Changelog

🚀 Enhancement

  • @theme-ui/color-modes, @theme-ui/css, gatsby-plugin-theme-ui
    • Refactor color objects and expose default colors as a mode #1639 (@fcisio)

🐛 Bug Fix

Authors: 2

@vercel
Copy link

vercel bot commented Apr 12, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/systemui/theme-ui/56FRK8hTMkfka8t8CmLKjwnorgPQ
✅ Preview: https://theme-ui-git-fork-fcisio-immutable-colormodes-object-systemui.vercel.app

@lachlanjc
Copy link
Member

Can you provide some more context on why you're changing this/what you're trying to do with exposing this object? I don't understand the intended effect here yet

@fcisio
Copy link
Collaborator Author

fcisio commented Apr 13, 2021

Sure!

The main use case for the colors && rawColors object, is for them to be used like this:

<div sx={{ boxShadow: (theme) => `0 0 4px ${theme.colors.primary}` }} />

 Which is why the object mutates in the first place.

Now, if the use case is to access the object outside of the sx prop, that's when we face some issues.


Here are two states of the rawColors object.
A: the initial shape of the object (the colorMode has not been changed with setColorMode or useColorSchemeMediaQuery)
B: the mutated shape of the object (the colorMode has been changed with setColorMode or useColorSchemeMediaQuery)

A

colors: {
  background: 'white',
  modes: {
    dark: { background: 'black' }
  }
}

B

colors: {
  background: 'black', // mutated
  modes: {
    dark: { background: 'black' }
  }
}

This means that doing something as simple as this becomes very inconvenient.

Object.entries(allColorModes).map(([mode, values]) => ({
  <Button
    sx={{ bg: values.background, color: values.text }}
    onClick={() => setColorMode(mode)}
  >
    {mode}
  </Button>
}))

There is no direct way to include the default colors (not explicitly in modes). If we do include it manually, all it takes is for the color mode to change and the whole thing breaks.

I can think of countless other use cases like this, which is why I think the PR would be useful.


A simple workaround that I've been doing is to create the modes outside the theme, and create the allColorModes on the user side:

const allColorModes = {
  light: { background: 'white' }
  dark: { background: 'black' }
}

// in the theme

{
  colors: {
    ...allColorModes[initialColorModeName],
    modes: omit(allColorModes, [initialColorModeName])
  },
  allColorModes // add it here
}

It works well, I just think it's unnecessary overhead for common use cases.

Sorry for the long chapter, I like to be very thorough. Just let me know if it gets tiresome.

@lachlanjc
Copy link
Member

lachlanjc commented Apr 13, 2021

No I appreciate the thoroughness! Is there an ultimate usecase for this, beyond docs websites where you want to show these color modes? I’ve always solved this by importing the theme directly & using colors from there instead of via context, since the theme file is “the source with all the colors” whereas the theme context are “the current colors”. I’m worried exposing colors in another place will be confusing, especially for beginners.

@fcisio
Copy link
Collaborator Author

fcisio commented Apr 13, 2021

I just think that the context is the most convenient way to get the object. Sure it could simply be imported from the file, but that file could be pretty far relative to the import.

I remember when I first started using Theme UI, the mutability of the colors was what confused me haha.

At least with allColorModes, we could say that it's where you get the initial source of truth.

My use cases might be advanced-ish since I mostly use Theme UI on creative projects (a lot on interactions, motion, and theme-related stuff).

As for use cases, there is the one above, but some others would be:

  • Using a tool like GSAP to tween between the mode changes (which I just did)
  • Creating themed sections
  • Listing all the modes
  • ...

If this PR doesn't go through, we could potentially edit the docs to explicitly say that the color objects are mutated upon color mode change.
Then maybe also provide a recipe to create an immutable object manually.

@lachlanjc
Copy link
Member

Interesting. I think the docs change you mention should happen regardless (PR welcome, anyone!). I feel like I'm missing something obvious here, but why does rawColors change with the color mode? I thought colors changed (as it should), but rawColors was that immutable object from the theme? cc @hasparus

@hasparus
Copy link
Member

hasparus commented Apr 15, 2021

rawColors changes because this is supported

const { theme } = useThemeUI();

const currentPrimaryVar = theme.colors.primary, // var(theme-ui-colors-primary)
const currentPrimaryRaw = theme.rawColors.primary; // e.g. "#fff"

return <ExternalComponentWhichDoesntSupportCSSVars color={currentPrimaryRaw} />

This could also be used inside of sx, e.g. by passing a color to a helper from polished.

TBH I'm not a fan of mutation in general and I don't really like the current API, but it was the most practical for now.


Regarding the PR:

I'd like to keep theme as close to System UI Theme specification, so I'm a bit skeptical about adding it to the theme itself.
But

  • the villain who mutates the theme is ColorModeProvider from @theme-ui/color-modes package
  • it also adds colorMode and setColorMode

What do you think about adding allColorModes to the Theme UI context in ColorModeProvider? We wouldn't be able to use them in sx, but I don't think we have many use cases for it.

@hasparus hasparus added the enhancement New feature or request label Apr 15, 2021
@fcisio
Copy link
Collaborator Author

fcisio commented Apr 15, 2021

@hasparus Not sure about where the object would end up if put in ColorModeProvider?

  • colorMode
  • setColorMode
  • ? allColorModes
  • theme

Regardless, I just realized that I'm a big noob. I think I designed this thing backward.

We could keep the logic behind adding the default colors within a mode initialColorModeName || '__default', but simply inject it in the current rawColors.modes.

rawColors: {
  primary: 'white',
  modes: {
    // __default: { primary: 'white' },
    light: { primary: 'white' },
    dark: { primary: 'black' },
  }
}

The modes themselves aren't mutated as of now, so it makes sense.
That way we say: hey you can use the current colors with theme.rawColors.primary, and you can access the modes colors with: theme.rawColors.modes['light', 'dark', '__default', ...].primary

This approach achieves the same result as allColorModes and doesn't pollute the context.

What do you guys think? 🤷‍♂️

@fcisio
Copy link
Collaborator Author

fcisio commented Apr 16, 2021

Alright! I reverted a little bit. Here are the main changes:

Removed the allColorModes object in favor of adding the default colors as a mode inside rawColors.

Details in my last comment.

New rawColors:

ScreenShot 2021-04-15 at 9 18 22 PM

Removed the modes from the colors object.

The values of the modes were CSS Vars that didn't exist. It's probably better to remove them, as its a bit confusing.

FROM:

ScreenShot 2021-04-15 at 9 23 59 PM

TO:

ScreenShot 2021-04-15 at 9 18 36 PM

I fed the outer.theme into the colors object instead of the colorMode aware one.

It didn't make sense that this object would be mutated since its values (CSS Custom Properties) never change.

@hasparus
Copy link
Member

hasparus commented Apr 16, 2021

Okay, wow. This is better than anything I thought of.

Yeah, keeping all modes in rawColors.modes makes a lot of sense.

@fcisio
Copy link
Collaborator Author

fcisio commented Apr 16, 2021

@hasparus I applied both suggested changes. Love these ideas.

I added rawColors as well inside useMemo.

Comment on lines 23 to 29
const root = render(wrapRootElement({ element: <Consumer /> }, {}))
expect(context.theme).toEqual({
colors: {},
rawColors: {},
styles: {
pre: {},
},
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand the logic of this test completely.

I adapted it because it was failing, but I'll let you guys see if that's actually what we want.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 It tests... just if the context provider is used by wrapRootElement? Okay, let's leave it.

@fcisio fcisio changed the title Create an immutable "allColorModes" object in Theme UI context Refactor color objects and expose default colors as a mode Apr 16, 2021
Copy link
Member

@lachlanjc lachlanjc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fantastic, thank you for your iterations! I think the one thing left here is docs: I'm not sure the best place, maybe just a quick section under useThemeUI on the Hooks page?

@fcisio
Copy link
Collaborator Author

fcisio commented Apr 19, 2021

I drafted some docs in my last commit.
Figured the Color Modes page was the best place to put it.

Not too sure about how comprehensible I was able to make it. Feel free to edit until it looks right 👌

Copy link
Member

@lachlanjc lachlanjc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great start, thank you! Included a few suggestions to make it clearer

Comment on lines 113 to 117
const context = useColorMode(null)
const { setColorMode } = context
const { rawColors: colors } = context.theme

return Object.entries(colors?.modes).map(([mode, values]) => ({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this code is a bit confusing, especially in renaming rawColors to colors when this section is differentiating them. It also should be the useThemeUI hook here, right? Let's do this:

  const { theme: { rawColors }, setColorMode } = useThemeUI()

  return Object.entries(rawColors?.modes).map(([mode, values]) => ({

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, it was copy-paste accident!


<Note>

For more info, [see the migration guide](/migrating/#breaking-changes)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's update with a specific section, so that this line doesn't become out of date

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the note completely, the info on the page should be sufficient.

And I think that updating the docs with the content of the migration guide should be a PR of its own.


#### With Theme UI context

Use the `useThemeUI` hook to access the context object directly in a component.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's link the hook name to: https://theme-ui.com/hooks#usethemeui

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, I linked both useThemeUI & useColorMode

@hasparus
Copy link
Member

Do you guys feel we should release it as minor or patch update?

@fcisio
Copy link
Collaborator Author

fcisio commented Apr 27, 2021

Do you guys feel we should release it as minor or patch update?

Minor might make more sense especially since we're introducing a bunch of docs content.
But it kinda qualifies for either 🤷‍♂️

If we include my other PR #1685 in the release, then it would definitely be a patch

@hasparus
Copy link
Member

Since this PR theme.colors.modes are never accessible from the context, but required to configure color modes.

I'm considering introducing separate types for the theme received from the user and the theme given through context. What do you think about it? (I don't mean in this PR)

@fcisio
Copy link
Collaborator Author

fcisio commented Apr 28, 2021

I reworked the docs, with your comments in mind!

@hasparus

I'm considering introducing separate types for the theme received from the user and the theme given through context.

Not sure I follow, but if you are talking about TS Typing it sounds like a good idea.


I also started to think about a way to handle the colors without mutability. This might impact what you decide to do regarding types.

I would need to study the current setup, but I was thinking:

  • Refactor getColor() to be aware of the mode, then modes[colorMode][color]
  • Change the shape of the colors object (decouple colors from modes + allow color ref in mode keys)
colors: {
  light: '#FFFFFF',
  dark: '#000000',
  blue: '#3498DB',
  red: '#E74C3C',
},
initialColorModeName: 'light',
modes: {
  light: {
    bg: 'light',
    text: 'dark',
    primary: 'blue',
  },
  dark: {
    bg: 'dark',
    text: 'light',
    primary: 'red',
  },
}

Co-authored-by: Piotr Monwid-Olechnowicz <hasparus@gmail.com>
@hasparus
Copy link
Member

Huge thanks for your work here, Francis!

@hasparus hasparus merged commit 01e73cd into system-ui:develop Apr 28, 2021
@hasparus hasparus added the prerelease This change is available in a prerelease. label Apr 28, 2021
@hasparus
Copy link
Member

hasparus commented May 5, 2021

🚀 PR was released in v0.8.0 🚀

@hasparus hasparus added released This issue/pull request has been released. and removed prerelease This change is available in a prerelease. labels May 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants