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

feat: Add support for slot level variant overrides #82

Merged
merged 3 commits into from
Sep 2, 2023

Conversation

mskelton
Copy link
Collaborator

Fixes #48

Description

This adds support for slot level variant overrides. This is useful in a variety of situations:

  1. Component libraries that provide slot level class name functions:
    const {base,tab} = tv({...})
    
    <Tabs className={() => base()}>
      <Tab className={({ isSelected }) => tab({isSelected})}>
        Settings
      </Tab>
    </Tabs>
  2. Reusing styles for vary similar components.
    const {base,item} = tv({...})
    
    <Nav className={base()}>
      <NavItem className={item({isActive: activeItem === 'foo'})}>foo</NavItem>
      <NavItem className={item({isActive: activeItem === 'bar'})}>bar</NavItem>
    </Nav>

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Follow the Style Guide.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).

package.json Show resolved Hide resolved
@jrgarciadev
Copy link
Member

Amazing! Huge thanks @mskelton , could you please fix the conflicts?

@mskelton
Copy link
Collaborator Author

@jrgarciadev I've fixed the conflicts.

@jrgarciadev
Copy link
Member

@mskelton would you like to write a section about this in the docs? https://github.com/nextui-org/tailwind-variants-docs, in here: https://www.tailwind-variants.org/docs/slots

@mskelton
Copy link
Collaborator Author

@jrgarciadev PR opened with docs for this feature.

nextui-org/tailwind-variants-docs#13

@mskelton
Copy link
Collaborator Author

@jrgarciadev Can we get this PR merged as well as my docs PR?

@jrgarciadev
Copy link
Member

@tianenpang could you please take a look at this PR?

@jrgarciadev
Copy link
Member

@mskelton something that I just noticed is that the speed is lower than the current version, here's the benchmark with these changes

TV without slots & tw-merge (enabled) x 436,739 ops/sec ±0.40% (96 runs sampled)
TV without slots & tw-merge (disabled) x 511,653 ops/sec ±0.62% (98 runs sampled)
TV with slots & tw-merge (enabled) x 179,561 ops/sec ±3.74% (95 runs sampled)
TV with slots & tw-merge (disabled) x 195,324 ops/sec ±1.64% (90 runs sampled)
TV without slots & custom tw-merge config x 426,118 ops/sec ±0.61% (95 runs sampled)
TV with slots & custom tw-merge config x 210,646 ops/sec ±0.53% (98 runs sampled)
Fastest is TV without slots & tw-merge (disabled)

Here is the current one:
TV without slots & tw-merge (enabled) x 452,002 ops/sec ±0.28% - 6.57% (% speed increased)
TV without slots & tw-merge (disabled) x 558,383 ops/sec ±0.60% - 17.41% (% speed increased)
TV with slots & tw-merge (enabled) x 311,351 ops/sec ±0.68% (99 runs sampled) - 8.01% (% speed increased)
TV with slots & tw-merge (disabled) x 359,928 ops/sec ±0.81% (93 runs sampled) - 8.61% (% speed increased)
TV without slots & custom tw-merge config x 429,622 ops/sec ±1.60% (95 runs sampled) - 4.42% (% speed increased)
TV with slots & custom tw-merge config x 393,804 ops/sec ±0.84% (96 runs sampled) - 4.94% (% speed increased)

Fastest is TV without slots & tw-merge (disabled)

Could you please check why? 🙏🏻

Copy link
Member

@tianenpang tianenpang left a comment

Choose a reason for hiding this comment

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

LGTM, thanks to @mskelton for this awesome feature 🚀

@tianenpang
Copy link
Member

Hi @jrgarciadev I've made some optimizations to the performance, please try benchmarking again.

@jrgarciadev
Copy link
Member

Amazing the performance is even better than the current one, thank you @tianenpang 🙏🏻

@jrgarciadev jrgarciadev merged commit 9e6ed81 into nextui-org:main Sep 2, 2023
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.

Slot level variant overrides
4 participants