nandorojo / dripsy

🍷 Responsive, unstyled UI primitives for React Native + Web.
https://dripsy.xyz
MIT License
1.99k stars 77 forks source link

defaultVariant styles inherited in other variants #222

Closed jjenzz closed 11 months ago

jjenzz commented 1 year ago

if i add some buttons variants to my theme and declare one of these as the defaultVariant for my Button component, styles from the defaultVariant must be manually reset or overridden in my other variant options.

for example, take the following excerpt from my theme object:

  buttons: {
    medium: {
      borderRadius: "$xlarge",
      py: 14,
      px: "$8",
    },
    circle: {
      borderRadius: "$rounded",
      alignItems: "center",
      justifyContent: "center",
      width: 40,
      height: 40,
    },
  },

with the following component definition:

const Button = styled(Pressable, {
  themeKey: "buttons",
  defaultVariant: "medium",
})({
  alignSelf: "center",
});

when rendering <Button variant={'circle'} /> it will have the py and px padding from the medium variant. i have to add px: 0, py: 0 to my circle variant to prevent this.

i didn't expect it to inherit this way. it seems like it could be a bug because if i do not set a defaultVariant then no inheriting occurs. let me know if my expectations are misplaced here tho.

nandorojo commented 1 year ago

thanks for the issue! i see, you’re expecting variant || defaultVariant. i’ll look into this for you and see what’s up.

nandorojo commented 1 year ago

so defaultVariant currently functions the same way as passing a style directly to styled(View)(style). which is to say, it is inherited.

i can obviously see how that's confusing: default implies that it would be a fallback if no other variant is supplied. this was originally intended for making it easy to extend a button and things like that with inheritance in mind. the idea was that components should solve narrow use cases and be easily extendable from their base version.

there is also a defaultVariants field and variants prop with the same behavior, for reference.

given the confusing behavior of defaultVariant, i'd have to think about how to adjust it. initialVariant would have been a better name given it's behavior. i'll give this some more thought

jjenzz commented 1 year ago

this was originally intended for making it easy to extend a button and things like that with inheritance in mind

if inheritance is my intention, it seems something like this would be simple enough and more explicit:

buttons: {
  medium: {
    // an object i create for common styles
    ...commonButtonStyles,
    px: 10,
    py: 10,
  },
  circle: {
    ...commonButtonStyles,
    borderRadius: 9999,
  }
}

the idea was that components should solve narrow use cases and be easily extendable from their base version.

i understand. although, i'd likely reach for the above for inheritance. otherwise, the theme has no control over whether the defined styles are inherited or not, it's up to the dev who implements the theme (whether they choose to set defaultVariant or not).

also, how would i define a variant prop that is optional?

if you think inheritance would be better as part of the api, maybe something like this could work?

buttons: {
  // a reserved key name for styles that get applied to `styled` body (styled(View)(style))
  style: {
  },
  // `defaultVariant` can be one of these to compose with base styles and make variant optional
  medium: {
    px: 10,
    py: 10,
  },
  circle: {
    borderRadius: 9999,
  }
}
nandorojo commented 1 year ago

The reason defaultVariant was added originally was, React Native has no global/default styles. This only really matters for components like Text. For example, I want all my Text components to use a default fontSize and fontFamily/fontWeight.

That's why, by default, the Text component from dripsy extends the theme.text.body, H1 extends theme.text.h1, etc. All of their defaultVariants are set to those values.

const theme = makeTheme({
  text: { body: { fontFamily: 'root', color: 'white' } }
})

All you have to do is that, and then you can import { Text } from 'dripsy' and have "default styles" applied to it. For Web, this is easy with CSS, but for React Native, it's pretty unusual.

The thing is, I wouldn't want that defaultVariant to necessarily restrict someone from adding more styles/variants to the Text. And since I've relied on the existing behavior so much already, I'd have to give some serious thought to changing it.

if you think inheritance would be better as part of the api, maybe something like this could work?

The style option doesn't exactly fit the case of default variants for h1 and body; we want theme.text to have multiple base options.

It's tricky overall, and I wish I'd known more of the API limitations at the time.

I have to admit, I've found myself using the variant prop somewhat infrequently over time. I prefer to see the styles closer to the components.

What if we added an fallbackVariant option, which gets overridden by variant used on the component itself? It's a little confusing to add more to the API, but I think this more clearly states that variant will "fallback" if nothing is provided.

jjenzz commented 1 year ago

I have to admit, I've found myself using the variant prop somewhat infrequently over time. I prefer to see the styles closer to the components.

i was also unsure about declaring styles in theme at first as I am used to stitches where variants are declared as part of the component, however, your approach has opened my eyes somewhat in that it could allow designers to more easily have control over visual styles. the components themselves could handle functional styles only.

i've been trialing going all in on this approach and enjoying it thus far!

What if we added an fallbackVariant option, which gets overridden by variant used on the component itself? It's a little confusing to add more to the API, but I think this more clearly states that variant will "fallback" if nothing is provided.

this would be super helpful and would suit my use-case perfectly 🙏