-
-
Notifications
You must be signed in to change notification settings - Fork 49
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
Add support for color objects with nested keys to processThemeColors
via new flattenThemeColors
function
#307
Add support for color objects with nested keys to processThemeColors
via new flattenThemeColors
function
#307
Conversation
🦋 Changeset detectedLatest commit: c1ad3a7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
if (typeof value === 'string') { | ||
memo[newKeys.join('-')] = value; | ||
} else { | ||
flattenColors(value, newKeys, memo); |
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.
Is there a need for this to be recursive (can you provide an example)?
If not, I would like to simplify this to something like...
return fromEntries(entries(colors).flatMap(([color, value]) => {
if (typeof value === 'string') {
return [color, value];
} else {
return entries(value).map(([shade, value]) => {
return [`${color}-${shade}`, value]
})
}
}))
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 use a similar optimization in my active PR:
I added support for recursion so if anyone wants to pass in an object that contains multiple levels, they don't have to do the flattening themselves, e.g.:
const colors = {
gray: {
cool: {
50: '#f8fafc',
// ...shades
},
warm: {
50: '#fafaf9',
// ...shades
},
}
}
This same idea is also actively being discussed for inclusion in CSS (primarily for use in CSS libs like Tailwind CSS) here: w3c/csswg-drafts#9992
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.
@techniq If you would still rather this only support one level, I can refactor this. Just lmk
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'm fine with the recursion. A lot of the times I think of -content
as it's own color and reference it explicitly (primary-content
, etc) but supporting it in the same object is nice.
Care to add a test for flattenColors()
(create theme.test.ts
) to capture these use cases? I need to add tests for more of the theme functions (processThemeColors()
, etc), but this would be a good and simple start. Thanks!
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.
Tests added for both flattenThemeColors
and processThemeColors
, and support added for 'DEFAULT'
shade, matching Tailwind's nomenclature, though this will be usable by any CSS library.
UPDATE: Renamed flattenColors
to flattenThemeColors
to make more specifically descriptive
processThemeColors
processThemeColors
via new flattenThemeColors
function
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.
Looks good. Thanks!
This will resolve "Support passing color objects from tailwind" from #250