class_variants icon indicating copy to clipboard operation
class_variants copied to clipboard

PR Suggestions

Open willcosgrove opened this issue 2 years ago • 2 comments
trafficstars

I've got a couple of local changes on my fork of class_variants that I thought might be good candidates for being merged in to the real thing. I've also got some changes that I'm not sure would be a good fit, but I wanted to run them by you to see what you thought.

First: I've added support for compound variants. It adds a little bit of a performance hit, but it's a feature I've needed in almost every use case I have of this tool, so it seems necessary, at least for me. It involved a two part change, which you could choose to add either one or both of. The first is allowing a variant to resolve to another ClassVariants::Instance object, which would call render on that instance, passing in the same render options. That alone allows you to support any kind of compound variant.

The second half of that change is making it less cumbersome to define class variants with compound variants. This is going to be more debatable in terms of what constitutes a good developer experience. But this is the syntax I have now:

badge_classes = ClassVariants.build("inline-flex items-center font-medium",
  variants: {
    size: {
      base: "px-1 py-0.5 text-sm",
      lg: "px-3 py-1 text-base",
    },
    shape: {
      pill: "rounded-full",
      rounded: "rounded",
    },
    remove: { # This is a boolean variant specifying whether or not the badge has a remove (x) button
      size: {
        base: "pl-1 pr-0.5",
        lg: "pl-2.5 pr-1",
      }
    },
  },
  defaults: { size: :base, shape: :pill, remove: false }
)

This is a pretty complex case, as it involves both a boolean variant combined with a compound variant, which takes some work to detect. But all that work is done on object initialization so it doesn't affect render speed.

Recall that the boolean variant allows you to specify a variant as a string, instead of as a hash of option->string pairs. Because of this, it's not obvious whether

remove: {
  size: {
    base: "pl-1 pr-0.5",
    lg: "pl-2.5 pr-1",
  }
}

Should be expanded as:

remove: {
  size: ClassVariants.build(variants: {
    base: { true => "pl-1 pr-0.5" },
    lg: { true => "pl-2.5 pr-1" },
  })
}

Or as (which is actually what my intention is in the example above):

remove: ClassVariants.build(variants: {
  size: {
    base: "pl-1 pr-0.5",
    lg: "pl-2.5 pr-1",
  }
})

The way my current implementation deals with this problem is by checking the hash's keys to see if they are all top level variants (in this case size is a known variant). If they are all known variants, then it assumes the hash is actually a compound variant and not a hash of variant options.

So, I'm not super happy with the complexity around resolving these cases, but I also don't like having to nest more ClassVariants.build in my variant definitions so 🤷‍♂️

Maybe there is a better alternative that introduces less ambiguity.


The other thing I have in my fork right now is a caching layer that caches the rendered class strings for the provided options. This roughly doubles the performance, at the cost of memory. This could be an issue if you have extremely large class variant objects with many variants and many options per variant. It's the kind of change I'm not sure is going to be suited to all use cases, so I'm not sure it's a good add. You could include it but make it configurable, but that adds complexity too. Ultimately I'm just not sure if the speed up is worth the hassle.

willcosgrove avatar Feb 07 '23 16:02 willcosgrove

@willcosgrove have you seen how other javascript libraries do this for inspiration:

  • https://github.com/joe-bell/cva
  • https://stitches.dev/docs/variants#compound-variants

I suggest creating a new key compoundVariants

badge_classes = ClassVariants.build("inline-flex items-center font-medium",
  variants: {
    size: {
      base: "px-1 py-0.5 text-sm",
      lg: "px-3 py-1 text-base",
    },
    shape: {
      pill: "rounded-full",
      rounded: "rounded",
    },
    remove: "",
  },
  compoundVariants: [
    {
      size: 'base',
      remove: true,
      class: 'pl-1 pr-0.5'
    },
    {
      size: 'lg',
      remove: true,
      class: 'pl-2.5 pr-1'
    }
  ],
  defaults: { size: :base, shape: :pill, remove: false }
)

I would also love compound variants support. (I'd attempt to do this but my ruby is noob (still learning) since I've mainly only ever used javascript.

So the code up top would be used as so

output = button_classes.render(shape: :pill, size: :base, remove: true)
# output 
# "px-1 py-0.5 text-sm rounded-full pl-1 pr-0.5"

Compound variants should ALWAYS go at the end.

Now since we are using tailwind we will have conflicting classes luckily we can solve this by using another gem tailwind_merge. This is how we do it at my current company

ErnestoR avatar Feb 16 '23 17:02 ErnestoR

Hey guys, Sorry for the late reply.

I'm all in for adding compound variants. I'd love to merge in a PR if we had one.

Regarding the performance suggestion, my thoughts are in the air too @willcosgrove. It would be cool to have a configurable caching layer as long as it doesn't bring in a lot of complexity. Otherwise... yeah, I'm all in!

adrianthedev avatar Feb 20 '23 08:02 adrianthedev