-
-
Notifications
You must be signed in to change notification settings - Fork 5
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
Comments
@willcosgrove have you seen how other javascript libraries do this for inspiration: I suggest creating a new key 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 |
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! |
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:
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
Should be expanded as:
Or as (which is actually what my intention is in the example above):
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.
The text was updated successfully, but these errors were encountered: