feat(Theme): new component

[Bobakanoosh]

🔗 Linked issue

Resolves #4250

❓ Type of change

• [x] 📖 Documentation (updates to the documentation or readme)
• [ ] 🐞 Bug fix (a non-breaking change that fixes an issue)
• [ ] 👌 Enhancement (improving an existing functionality)
• [x] ✨ New feature (a non-breaking change that adds functionality)
• [ ] 🧹 Chore (updates to the build process or auxiliary tools and libraries)
• [ ] ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

📚 Description

Detailed in the issue: https://github.com/nuxt/ui/issues/4250

Needing Review

This MR is still in draft mode, but I have an initial POC made. I was hoping to get feedback on the approach and the changes before applying them to all components, and writing documentation.

📝 Checklist

• [x] I have linked an issue or discussion.
• [ ] I have updated the documentation accordingly.

[Bobakanoosh]

@benjamincanac Any thoughts on this? I'd love to finish out the rest of this MR, just wanted to get your initial thoughts.

[Bobakanoosh]

Also, I think it would be best to externalize the composable to be consistent.

Done!

Wouldn't it be easier to inject the provided ui prop inside the ui computed like we do for the app config? 🤔 https://github.com/nuxt/ui/blob/v3/src/runtime/components/Button.vue#L106

My first thought was also to put it where we put app config. But the ui prop and the app.config.ts behave a bit differently (unfortunately), and I wanted it to function like the ui prop.

For example, applying the following in app.config.ts does not work:

button: {
   slots: {
       base: 'px-20'
   }
}

But applying the same thing via the ui prop does. This is because the app config value isn't getting sent into the tailwind merger. I'd consider this to be a worse DX than how the UI prop works, so instead I made sure the slots coming from UTheme get tailwind merged with the ui prop.

In other words, if I wanted to change the base padding of all buttons in an area, I would have to resort to px-20!, which is less than desirable.

[Bobakanoosh]

@benjamincanac Whenever you have a chance could you re-review above? Would love to finish this PR off

[benjamincanac]

@Bobakanoosh This is not entirely true, the app config value is merged with the config. For the button case, doing this does not work because the padding is defined inside the size variant: https://github.com/nuxt/ui/blob/v3/src/theme/button.ts#L41

export default defineAppConfig({
  ui: {
    button: {
-     slots: {
-       base: 'px-20'
-     },
+     variants: {
+       size: {
+         md: {
+           base: 'px-20'
+         }
+       }
+     }
    }
  }
})

The ui prop overrides the slots after the variants have been computed.

[Bobakanoosh]

@benjamincanac

The ui prop overrides the slots after the variants have been computed.

Which is how I would want and expect the Theme component to function as well. I wouldn't want to have to modify the variant as you're showing in that example.

Having the theme component function exactly how the ui prop on components functions feels most intuitive to me.

[Bobakanoosh]

@benjamincanac Whenever you have a chance could you check my reply above?

[Bobakanoosh]

@benjamincanac Now with v4 out (congrats!) can we get this MR moving again?

[benjamincanac]

@Bobakanoosh Sure! I'm sorry I have some delay on PR reviews due to the focus on v4 the previous months.

First, we can start by moving this branch to v4 and I agree that this should behave like the ui prop!

[Bobakanoosh]

@benjamincanac All good on the delay.

Ready for review here, a few notes:

• I cant see why the deploy is failing despite having a Vercel account, it shows a 404.
• I had 3-4 tests failing locally due to unrelated changes, but I dont see an action to run those tests so not sure if they're happening in CI.
• It says theres merge conflicts but my fork says its synced with latest v4 so confused there..

[Bobakanoosh]

@benjamincanac any updates here

[benjamincanac]

@Bobakanoosh Sorry I haven't taken the time to review this properly but let's try to finish this for the next minor.

First of all, would you mind fixing the conflicts?

I think the composable should take the entire props object as second parameter to match other composables, it doesn't make much sense to pass () => ({ slots: props.ui }) every time and it should return the direct object like props.ui.

I'm still unsure about the naming, maybe useComponentUI?

const uiProp = useComponentUI('button', props)

:class="ui.label({ class: uiProp.label, active })"

[Bobakanoosh]

Will make those changes today/tomorrow