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

(fix): responsive variants for base when slots are present #202

Merged
merged 2 commits into from
Nov 12, 2024

Conversation

w0ofy
Copy link
Contributor

@w0ofy w0ofy commented Jun 17, 2024

Description

cc @codecaaron

While trying to use responsiveVariants with slots present, any responsive props passed were not output in the component's classname.


What is the purpose of this pull request?

Ensure that responsive classnames are applied to base correctly when slots are also used.

  • Bug fix

Here's a minimal code example of the issue:

// example
const button = tv(
    {
        base: 'inline-flex cursor-pointer gap-2 ...',
        slots: {
            icon: 'w-4 h-4 aspect-square pointer-events-auto',
        },
        variants: {
            size: {
                xs: 'text-xs',
                sm: 'text-sm',
                md: 'text-md',
            },
        },
    },
    { responsiveVariants: true },
);

const Button = ({ children, icon: Icon, ...rest }) => {
        const { base, icon } = button(rest)
        return (
            <button {...rest} className={base()} ref={ref}>
                {children}
                {Icon && <Icon className={icon()} />}
            </button>
        );
    };

// responsive variants won't work as expected here. the output classname of this variant will result in undefined
<Button size={{ initial: 'xs', sm: 'sm' }} />

@@ -235,7 +235,10 @@ export const tv = (options, configProp) => {
if (screenValues.length > 0) {
screenValues.push(value);

return screenValues;
if (slotKey === "base") {
return screenValues.join(" ");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ensures line 266 is evaluated and returns the variantValue as a string

@codecaaron
Copy link

codecaaron commented Jun 17, 2024

Note for context the issue is related to the logic here:
https://github.com/nextui-org/tailwind-variants/pull/202/files#diff-bfe9874d239014961b1ae4e89875a6155667db834a410aaaa2ebe3cf89820556L260-R269

      for (const variant in variants) {
        const variantValue = getVariantValue(variant, variants, slotKey, slotProps);
        const value =
          slotKey === "base" && typeof variantValue === "string"
            ? variantValue
            : variantValue && variantValue[slotKey];
        if (value) {
          result[result.length] = value;
        }
      }

The return value of getVaraintValue when slotKey === "base" can also be an array of responsive classnames - which fails the boolean in the above logic, the right hand side of the ternary then attempts to get the value of the key base on the array, returning undefined erroneously. It's unclear if the object case of this ternary is required for functionality in different configurations - the current suggested change simply ensures that the return type of getVariantValue doesn't fail this check and remains agnostic to other behaviors that may rely on the ternary logic here.

@jrgarciadev
Copy link
Member

@tianenpang please check this

@tianenpang
Copy link
Member

Hi @jrgarciadev LGTM 🚀 also test case added 🧪 and thanks to @w0ofy for the fix 🙏

@w0ofy
Copy link
Contributor Author

w0ofy commented Aug 23, 2024

👋 Just checking in, when can we expect this to be merged and released?

@jrgarciadev jrgarciadev enabled auto-merge (squash) November 12, 2024 11:39
@jrgarciadev jrgarciadev disabled auto-merge November 12, 2024 11:39
@jrgarciadev jrgarciadev merged commit 02eb717 into nextui-org:main Nov 12, 2024
3 of 4 checks passed
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

Successfully merging this pull request may close these issues.

4 participants