-
Notifications
You must be signed in to change notification settings - Fork 678
Description
Bringing across a set of comments I had made against this PR.
There is a fair case that it would make sense for the sx
prop to only support CSS properties, and that was @jxnblk's original thought too. Especially so if support is added to reference theme values within CSS shorthand.
The other thought that came out of this was then how to handle variants. It would mean that variant
could no longer be accessed within the sx prop.
However, perhaps that's not too much of a problem. One thing I've found is that it quickly breaks down once the variant needs to affect multiple elements at once.
So in aiming to define a consistent API across a component library, I opted instead to always provide a variant prop (when needed), that way I can handle both cases. The problem is then a source of confusion around having the two options available. I haven't yet found a nice, (ideally declarative) approach to handling these kinds of variants. What are your thoughts?
In the world of the sx prop only allowing CSS properties, the variant API could be built around the variant prop and also better accomodate the ability to cover multiple elements at once.
Here's an example of a basic idea:
const variants = {
primary: {
wrapper: {
bg: 'primary',
},
heading: {
color: 'white',
},
subtitle: {
color: 'textWhiteMediumEmphasis',
},
},
secondary: {
wrapper: {
bg: 'none',
},
heading: {
color: 'text',
},
subtitle: {
color: 'textMediumEmphasis',
},
},
};
const CardHeader = ({
title,
subtitle,
variant,
...props
}) => {
return (
<Flex
{...props}
sx={{
borderTopLeftRadius: 'large',
borderTopRightRadius: 'large',
...variants[variant].wrapper,
}}
>
<Box sx={{ flex: 1 }}>
<Heading
as="h6"
sx={variants[variant].heading}
>
{title}
</Heading>
{subtitle && (
<Text
sx={variants[variant].subtitle}
>
{subtitle}
</Text>
)}
</Box>
</Flex>
);
};