-
Notifications
You must be signed in to change notification settings - Fork 672
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
Refactor color objects and expose default colors as a mode #1639
Conversation
Pull changes
sync fork
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/systemui/theme-ui/56FRK8hTMkfka8t8CmLKjwnorgPQ |
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 |
Sure! The main use case for the <div sx={{ boxShadow: (theme) => `0 0 4px ${theme.colors.primary}` }} />
Now, if the use case is to access the object outside of the Here are two states of the Acolors: {
background: 'white',
modes: {
dark: { background: 'black' }
}
} Bcolors: {
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>
}))
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 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.
|
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. |
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
As for use cases, there is the one above, but some others would be:
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. |
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 |
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 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
What do you think about adding |
@hasparus Not sure about where the object would end up if put in ColorModeProvider?
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 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. This approach achieves the same result as What do you guys think? 🤷♂️ |
Alright! I reverted a little bit. Here are the main changes: Removed the
|
Okay, wow. This is better than anything I thought of. Yeah, keeping all modes in rawColors.modes makes a lot of sense. |
@hasparus I applied both suggested changes. Love these ideas.
|
const root = render(wrapRootElement({ element: <Consumer /> }, {})) | ||
expect(context.theme).toEqual({ | ||
colors: {}, | ||
rawColors: {}, | ||
styles: { | ||
pre: {}, | ||
}, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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?
I drafted some docs in my last commit. Not too sure about how comprehensible I was able to make it. Feel free to edit until it looks right 👌 |
There was a problem hiding this 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
const context = useColorMode(null) | ||
const { setColorMode } = context | ||
const { rawColors: colors } = context.theme | ||
|
||
return Object.entries(colors?.modes).map(([mode, values]) => ({ |
There was a problem hiding this comment.
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]) => ({
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
463a3df
to
3751b7b
Compare
Do you guys feel we should release it as |
Minor might make more sense especially since we're introducing a bunch of docs content. If we include my other PR #1685 in the release, then it would definitely be a patch |
Since this PR 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) |
I reworked the docs, with your comments in mind!
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:
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>
Huge thanks for your work here, Francis! |
🚀 PR was released in |
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.
This object could be used in many ways, but here is a simple example:
Map modes and outputs themed buttons
Version
Published prerelease version:
v0.8.0-develop.0
Changelog
🚀 Enhancement
@theme-ui/color-modes
,@theme-ui/css
,gatsby-plugin-theme-ui
🐛 Bug Fix
Authors: 2