-
Notifications
You must be signed in to change notification settings - Fork 673
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 combining multiple variants #586
Conversation
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 looking great so far! I think since this shouldn't introduce any breaking changes, I'd like to get this out after v0.3
I need a bit of technical help finishing the css support but it's 90% ready to go.
Based on this comment from #403, let me know if I can point you in the right direction anywhere -- at a quick glance, it looks like it's not removing the variants
key from the style object, but can dig in later
08b216a
to
9d5bad5
Compare
This would be amazing. But maybe even better if we could tackle this issue through a ‘styled-components-modifiers’-like ‘modifiers’ property to keep the API clean/distinct? On one hand, having to repeatedly deep merge in your mind while developing is NOT fun. On the other, I’d imagine that 99% of the time the override-variant will be making a minor change to the styling—otherwise one would just have another ‘no-help-needed’ variant defined. So, to me, it feels like a we need a little door to reach through (should the need arise) and make small tweaks in an otherwise manageable way. In my mind, it would look something like this: <Text sx={{ variants: “text.h1”, modifiers: [‘bold’, ‘allCaps’, ‘primaryColor’, ‘etc.’] }} > ... I dunno; just spit ballin’. But would be very happy to see current PR land |
Sorry, could you clarify exactly what the difference you're proposing with the |
I guess, in my mind, it’s not different in the result. Which is why I think this PR is great! I just think the ergonomics are tougher when everything goes through one property. So, it’s different in execution, I guess—for my mental model, thinking of the ‘variant’ as the dominant style and the ‘modifiers’ as tweaks makes a lot of sense. I feel like I can see clearer lines of demarcation. Theming is hard; especially for beginners. I’m just throwin’ it out there. |
Hmm, that makes a little more sense. The example I use in the docs is title + caps, where they’re not overlapping, just combining. In practice styles will definitely overlap but I think it’s kind of the same thing as adding several classes in traditional CSS—but actually simpler because you don’t have a specificity battle after that. |
hum I'd still vote for |
Yeah, I agree. I like that pluralising |
This looks cool at first glance, but I think it is conceptually wrong 😞 1. Invalid variantsFor example, this is proposed here: buttons: {
// This alone is a button variant
primary: {
color: 'white',
bg: 'primary',
},
// This alone is a button variant
secondary: {
color: 'white',
bg: 'secondary',
},
// This alone is NOT a button variant! 😞
small: {
fontSize: 1
}
}, The Imagine in the future a design tool reading all buttons to display them, and then we have an unstyled I agree with @medelman17 when he said:
2. Array API and breakpointsUsing arrays is "kinda reserved" to breakpoints, @jxnblk mentioned here at #403 (comment) Could be confused with the future 3. Similar but competing APIsWhat to expect when combining both 4. Allows breaking the design systemThis very similar to class names. Meaning that everything is now possible, even invalid combinations: sx={{ variants: ['buttons.primary', 'cards.compact'] }}` To be honest, I don't think we should allow multiple variants on the My proposal
buttons: {
primary: {
color: 'white',
bg: 'primary',
},
secondary: {
color: 'white',
bg: 'secondary',
},
modifiers: {
size: {
small: { fontSize: 1 },
},
shape: {
rounded: { borderRadius: 4 },
pill: { borderRadius: 100 },
},
},
}, Usagesx={{
variant: 'buttons.primary'
modifiers: {
size: 'small',
shape: 'pill'
}
}} Creating variants with modifiersWe could abstract even more things to modifiers like colors and create a variant out of modifiers: primary: {
modifiers: { color: 'primary', shape: 'rounded' },
},
secondary: {
modifiers: { color: 'secondary', shape: 'rounded' },
},
pill: {
modifiers: { color: 'orange', shape: 'pill', size: 'small' },
}
modifiers: {
color: {...},
size: {...},
shape: {...},
} By doing that we can explicitly declare named variants and have the ability to modify then in an acceptable way. Responsivenesssx={{
variant: 'buttons.primary'
modifiers: [{ size: 'small' }, { size: 'large' }]
}} Advantages
|
Although I like your proposed solution @brunnolou. I'm still in favour of the multiple variants prop as it's much simpler to use. Not sure where the distinction of modifiers/tweaks and variants is but to me anything that changes a style on a component creates a stylistic variation of that component and can thus be called a variant. If you have a This syntax, for me at least, is easiest to grok: sx={{
variants: ['buttons.color.primary', 'buttons.size.small', 'buttons.shape.pill']
}} I do agree with @brunnolou that there could be some confusion with what to expect when the |
Theme as a stylesheet and variants as classNames 🤯
@cour64 one of my biggest issues with this is the power. it would easily be misused. We could see something like this:
Difference between tweak and variants
Following the BEM methodology, a "tweak" is the modifier:
A block or element in our context it's our components. IMO each variant alone defines the whole style of the component.
Reinforcing the design systemSo this is how I believe we could reinforce the design system constraints:
|
Since emotion supports composition by passing an array to the At the moment, I have to use the In the app I'm working on, I also have a one-off case where I need to compose various variants into a single variant. The workaround I've been using at the moment is by leveraging Emotion's
|
For what it’s worth, I published a ‘plugin’ of sorts that enables the above, BEM-like functionality by overloading either the sx or css prop. Find it 👉🏻 https://github.com/fabulascode/theme-ui-modifiers It’s early stages yet—edge cases haven’t been considered, etc.—but I like it’s functionality, the ergonomics, imho, are a better fit for my mental model—despite its obvious hacky-ness due to not being officially supported. 🤷🏻♂️ Here’s how it works generally (you can also use the css prop):
Would love to hear your thoughts and or ideas. |
@medelman17 I that's interesting, I like the ResponsivenessLike I mentioned before IMO using object notation would be better to be compatible with responsiveness. ThemingKeeping the modifiers at the component level we loose "themeability". Meaning that even if we change the theme, the modifiers will always remain the same. SuggestionUsing your idea maybe something like this could tackle these two issues:
// theme.js
buttons: {
primary: { ... },
secondary: { ... },
modifiers: {
size: {
small: { fontSize: 1 },
large: { fontSize: 3 },
},
shape: {
rounded: { borderRadius: 4 },
pill: { borderRadius: 100 },
},
},
},
function Button({ variant = 'primary', children, ...props }) {
// By variant and modifiers names.
const modifiers = useModifiers(`buttonns.${variant}`, props.modifiers);
return (
<ThemeUIButton variant={props.variant} sx={modifiers}>
{children}
</ThemeUIButton>
);
} <Button modifiers={{ shape: 'pill' }} />
<Button variant="secondary" modifiers={[{ size: 'small' }, { size: 'large' }]} /> That would be the general usage, but if we want a better API for the componensts, we could create a different Buttons component implementation to map props to modifiers (before passing to <Button variant="secondary" shape="pill" size="small" /> |
So, if you take a bit of a closer look, I think you’ll see that the approach I propose use object notation and solves for both responsiveness and theming—see the ‘bigger’ modifier.
Get Outlook for iOS<https://aka.ms/o0ukef>
…________________________________
From: Bruno Lourenço <[email protected]>
Sent: Wednesday, February 26, 2020 7:40:53 AM
To: system-ui/theme-ui <[email protected]>
Cc: Michael Edelman <[email protected]>; Mention <[email protected]>
Subject: Re: [system-ui/theme-ui] Add support for combining multiple variants (#586)
@medelman17<https://github.com/medelman17> I that's interesting, I like the useModifiers idea. I've just found two issues here.
Responsiveness
Like I mentioned before IMO using object notation would be better to be compatible with responsiveness.
E.g.: <Text variant="h1" modifiers={[{ weight: 'bold' }, { weight: 'normal' }]}.
Theming
Keeping the modifiers at the component level we loose "themeability". Meaning that even if we change the theme, the modifiers will always remain the same.
Suggestion
Using your idea maybe something like this could tackle these two issues:
1. Add modifiers to the theme
// theme.js
buttons: {
primary: { ... },
secondary: { ... },
modifiers: {
size: {
small: { fontSize: 1 },
large: { fontSize: 3 },
},
shape: {
rounded: { borderRadius: 4 },
pill: { borderRadius: 100 },
},
},
},
1. Get modifiers from the theme by variant and modifiers names.
function Button({ variant = 'primary', children, ...props }) {
// By variant and modifiers names.
const modifiers = useModifiers(`buttonns.${variant}`, props.modifiers);
return (
<ThemeUIButton variant={props.variant} sx={modifiers}>
{children}
</ThemeUIButton>
);
}
<Button modifiers={{ shape: 'pill' }} />
<Button variant="secondary" modifiers={[{ size: 'small' }, { size: 'large' }]} />
That would be the general usage, but if we want a better API for the componensts, we could create a different Buttons component implementation to map props to modifiers (before passing to useModifiers), to have sth liked this:
<Button variant="secondary" shape="pill" size="small" />
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#586?email_source=notifications&email_token=ADQ3VLMVQYWRKPE23W5EPA3REZPNLA5CNFSM4KH7TIN2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENACHTI#issuecomment-591406029>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ADQ3VLILD2LNF6354LWV2XLREZPNLANCNFSM4KH7TINQ>.
|
@jonavila I like the idea of having arrays in the
Are you saying that the variants should also be merged? <Box sx={[{ variant: 'buttons.primary' }, { variant: 'buttons.seconday' }]} Replace the If merged, then this would be possible (even though looks a bit weird): <Box sx={[{ variant: 'buttons.primary' }, { variant: 'buttons.sizes.large' }, { variant: 'text.caps' }]} Regarding the need for extending multiple variants, I can see the benefit, especially while defining the ConclusionI think both ideas are valid and coexist.
// theme.js
buttons: {
primary: { ... },
attention: {
variants: ['button.primary', 'text.caps'],
// Or
extends: ['button.primary', 'text.caps'],
},
}
// theme.js
buttons: {
primary: {
modifiers: { color: 'primary', shape: 'rounded' },
},
attention: {
extends: ['button.primary', 'text.caps'],
modifiers: { size: 'large' },
},
modifiers: {
color: { ... },
size: { ... },
shape: { ... },
},
} I think we should make clear this distinction: "multiple variants" vs "modifiers". |
@lachlanjc I think the problem with the implementation in the css package has to do with the handling of responsive styles here: theme-ui/packages/css/src/index.ts Line 206 in cf5b00a
where the variants array is being treated as a responsive style and only the first element of the Perhaps checking the key within the const responsive = styles => theme => {
const next = {}
const breakpoints =
(theme && (theme.breakpoints as string[])) || defaultBreakpoints
const mediaQueries = [
null,
...breakpoints.map(n => `@media screen and (min-width: ${n})`),
]
for (const key in styles) {
const value =
typeof styles[key] === 'function' ? styles[key](theme) : styles[key]
if (value == null) continue
if (!Array.isArray(value)) {
next[key] = value
continue
}
// check if the key is 'variants' and handle appropriately
if (key === 'variants') {
for (let i = 0; i < value.length; i++) {
const variant = flatten([
typeof value[i] === 'function' ? value[i](theme) : value[i],
])
if (variant == null) continue
if (!Array.isArray(variant)) {
next[key] = [...(next[key] || []), variant]
continue
}
// this handles responsive variants of with a 2d array so ['primary', ['small', 'medium']]
for (let i = 0; i < variant.slice(0, mediaQueries.length).length; i++) {
const media = mediaQueries[i]
if (!media) {
next[key] = [...(next[key] || []), variant[i]]
continue
}
next[media] = next[media] || {}
if (variant[i] == null) continue
next[media][key] = [...(next[media][key] || []), variant[i]]
}
}
continue
}
for (let i = 0; i < value.slice(0, mediaQueries.length).length; i++) {
const media = mediaQueries[i]
if (!media) {
next[key] = value[i]
continue
}
next[media] = next[media] || {}
if (value[i] == null) continue
next[media][key] = value[i]
}
}
return next
} and then applying the variants prop in the if (key === 'variants') {
// wrap val in an array and then flatten in case a single string is passed
const variants = css(
deepmerge.all(flatten([val]).map(v => get(theme, v)))
)(theme)
result = { ...result, ...variants }
continue
} The There might be a couple specificity issues with this implementation though. |
@brunnolou Yes, they will be merged because behind the scenes the sx prop is using emotion's css prop. So it will work exactly like the last example on this page: https://emotion.sh/docs/composition In that example, the composition overrides just the color property, but it leaves the background-color untouched. Per their docs:
|
Thanks for bringing up some of these ideas, all, but this level of discussion should probably happen in an issue rather than this PR. The proposal here is to add an additional |
The solution I proposed here #586 (comment) works for the most part but there's one issue I noticed. If a variant is used within a variant e.g. primary: {
p: 3,
fontWeight: 'bold',
color: 'white',
bg: 'primary',
borderRadius: 2,
},
disabled: {
variant: 'buttons.primary',
bg: 'muted',
} Then some style props could incorrectly be overwritten e.g. the output above merged would result in: {
p: 3,
fontWeight: 'bold',
color: 'white',
bg: 'muted',
borderRadius: 2,
variant: 'buttons.primary'
} the Hope this all makes sense. |
Thanks for the input @jxnblk, I stopped working on this since it seemed like it might go to waste. Will try to look back at getting this toward the finish line sometime soon. Even deciding that we don’t want to move forward with some of the other proposals here, glad we discussed them! |
I like some of the ideas with this but I definitely think it needs to be fleshed out a bit! It's a big conceptual change to the meaning of a variant, and I think it's overloading the variant API a bit unnecessarily. I don't think there is any reason why these kinds of variants couldn't just be handled in the component itself via props. I quite like the modifiers suggestion by @brunnolou #586 (comment) and funnily enough the The interesting thing is you can already implement most of this already outside of theme-ui and basically ignore the <button sx={{ variant: 'buttons.primary' }}>...</button> In this case the <button variant="buttons.primary">...</button>
<Button variant="secondary" shape="pill" size="small" /> Perhaps the combination of these hooks would be perfect: const Button = ({
variant,
shape,
size,
...props
}) => {
const modifierStyle = useModifiers('buttons', { variant, shape, size });
return ...;
}); Also this would allow you to easily style multiple elements with the Edit: I've just edited this with a revision to drop the |
Here's an example implementation of the idea above to play around with: https://codesandbox.io/s/theme-ui-hook-api-example-9nnmu?file=/src/index.js |
Closing since there are many other PRs for this functionality now, & the files have changed since the PR was made |
Inspired by #403, I've added support for combining multiple variants by passing
variants
an array. It's a new prop on components, or a property insidesx
.components
&css
.css
is failing because something about my implementation incss
doesn't work—to be honest, I have no idea what's wrong & would super appreciate some assistance there.