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

Add variant composition feature #1017

Closed

Conversation

rogermparent
Copy link

@rogermparent rogermparent commented Jun 20, 2020

This addition allows for variants defined in css style objects to be composed using
space-separated lists. Style objects will compose on top of each other left-to-right
(last one wins) before being fed into css.

The behavior of single variants is unchanged, but the implementation is now a
byproduct of the composition behavior. (i.e. we variant.split(' ') all
variants).

Also, to facilitate this change the merge function has been moved to css, as
its previous home core depends on css such that there would be a circular
dependency otherwise. It also just makes sense if we move this merge behavior
down to the level of the css package.

core now exports merge from css instead of actually hosting the function.
deepmerge has been removed from core and added to css as a result, as the
merge function is the only thing that consumes it in either package. All
previous behavior should still work completely fine.

The variant separator is defined as a global constant, so it can be changed if
we think something like a comma, semicolon, or plus would be more evocative. I
think space is best because it mimics CSS class behavior. We could also get a
little more advanced and make it configurable, but I don't think that's
necessary.

I haven't started on docs yet, I need to find where specifically this behavior should go.
Nor have I done the changelog, but that was just be not knowing about it until just now.

I'm excited to get this feature in and hopefully contribute more as I start using theme-ui more heavily!

This addition allows for variants defined in `css` to compose with each other.

To invoke the feature, specify a space-separated list of variants. Their style
objects will compose on top of each other before being fed into `css`.

The behavior of single variants is unchanged, but the implementation is now a
byproduct of the composition behavior. (i.e. we `variant.split(' ')` all
variants).

Also, to facilitate this change the `merge` function has been moved to `css`, as
its previous home `core` depends on `css` such that there would be a circular
dependency otherwise. It also just makes sense if we move this merge behavior
down to the level of the `css` package.

`core` now exports `merge` from `css` instead of actually hosting the function.
`deepmerge` has been removed from `core` and added to `css` as a result, as the
`merge` function is the only thing that consumes it in either package. All
previous behavior should still work completely fine.

The variant separator is defined as a global constant, so it can be changed if
we think something like a comma, semicolon, or plus would be more evocative. I
think space is best because it mimics CSS class behavior. We could also get a
little more advanced and make it configurable, but I don't think that's
necessary.
merge.all = <T = Theme>(...args: Partial<T>[]) =>
deepmerge.all<T>(args, { isMergeableObject, arrayMerge })

export const defaultBreakpoints = [40, 52, 64].map((n) => n + 'em')
Copy link
Author

Choose a reason for hiding this comment

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

Prettier wraps these single-parameter lambdas in parentheses for some reason. I can remove them if necessary.

Comment on lines 298 to 302
const styles = css(
merge.all(
...val.split(VARIANT_SEPARATOR).map(variants => get(theme, variants))
)
)(theme)
Copy link
Author

@rogermparent rogermparent Jun 20, 2020

Choose a reason for hiding this comment

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

This is where all the magic happens!

And by magic, I mean string.split and deepmerge without much else.

@danielattilasimon
Copy link

Amazing! Just yesterday I was wondering if variants composed in theme-ui, and was disappointed to find that they don't. Hopefully, that'll change soon ;-)

@@ -9735,7 +9735,7 @@ gatsby-admin@^0.1.58:
react-icons "^3.10.0"
strict-ui "^0.1.3"
subscriptions-transport-ws "^0.9.16"
theme-ui "^0.4.0-rc.0"
theme-ui "^0.4.0-alpha.3"
Copy link
Author

Choose a reason for hiding this comment

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

This just kinda happened. It can be removed to be more properly in scope.

@rogermparent
Copy link
Author

Looks like I pushed without checking if things broke outside the package! I'll get these tamed.

My bad, it worked on my end with lingering workspace deps and I didn't notice I
didn't actually add it.
Also includes Prettier changes.

Fixes tests that were previously broken.
@rogermparent
Copy link
Author

rogermparent commented Jun 20, 2020

Haaa, looks like I originally didn't actually add deepmerge to @theme-ui/css. Pretty solid confirmation I should go to bed and pick this back up tomorrow!

I also forgot to make the composition tolerate variants that aren't defined.

With these changes, all tests pass locally for me!

@jxnblk
Copy link
Member

jxnblk commented Jun 26, 2020

Sort of a duplicate of #586 and related to #832

@jxnblk jxnblk mentioned this pull request Jun 26, 2020
16 tasks
@rogermparent
Copy link
Author

Sort of a duplicate of #586 and related to #832

Thanks for pointing that out! I saw that PR before I started this and thought it was just an issue. Hopefully this will still be helpful as an example of an alternate implementation! At some point I'll try to take a look at both and contribute to the older PR if possible.

@jxnblk
Copy link
Member

jxnblk commented Jun 26, 2020

I've been trying to look at the current state of the other PR and a few other related pieces and figure out a direction forward to unblock this functionality (which I really would like to get in).

I like the new take on the API here, using spaces to separate multiple variants, which would still allow for responsive variant values and avoid the need to handle a variants property. Although I don't like that we never got the previous PR in, this one might be in better shape to merge, especially with the TS migration happening right now

@dburles dburles mentioned this pull request Jul 1, 2020
7 tasks
@lachlanjc lachlanjc added the enhancement New feature or request label Dec 3, 2020
@lachlanjc
Copy link
Member

Thank you so much for this contribution! Closing with #1226 as a successor though, since it doesn’t have conflicts/is more up-to-date. Super sorry this took so long with a sad ending, but we do appreciate it.

@lachlanjc lachlanjc closed this Dec 6, 2020
@rogermparent
Copy link
Author

Thank you so much for this contribution! Closing with #1226 as a successor though, since it doesn’t have conflicts/is more up-to-date. Super sorry this took so long with a sad ending, but we do appreciate it.

No worries! I'm happy that this feature is in regardless of who makes it- especially where I've been busy with work. If nothing else, I hope my approach added something to the discussion!

Good luck to everyone working on theme-ui, I'm looking forward to using these features in future projects!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants