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

PR Suggestions #5

Open
willcosgrove opened this issue Feb 7, 2023 · 2 comments
Open

PR Suggestions #5

willcosgrove opened this issue Feb 7, 2023 · 2 comments

Comments

@willcosgrove
Copy link
Contributor

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.

@ErnestoR
Copy link

ErnestoR commented Feb 16, 2023

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

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

@adrianthedev
Copy link
Contributor

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!

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

No branches or pull requests

3 participants