-
Notifications
You must be signed in to change notification settings - Fork 529
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(module)!: use tailwind-merge
for app.config
& move config to components & type props
#692
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
tailwind-merge
for app.config by moving to componentstailwind-merge
for app.config
& move config to components
tailwind-merge
for app.config
& move config to componentstailwind-merge
for app.config
& move config to components
@romhml @aditio-eka To let you know, I just merged
Long story short, instead of omitting |
tailwind-merge
for app.config
& move config to componentstailwind-merge
for app.config
& move config to components & type props
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @benjamincanac, I found a bug caused by this function. You can see the details here: #742. It's missing reactivity from the ui
prop and the class
attribute.
I suggest changing this source code to
type UIProps<T> = {
ui: Partial<T & { strategy: Strategy }>
}
export const useUI = <T>(key, $props: UIProps<T>, $config?: T, { mergeWrapper = false }: { mergeWrapper?: boolean } = {}) => {
const $attrs = useAttrs()
const appConfig = useAppConfig()
const ui = computed(() => {
return mergeConfig<T>(
$props.ui?.strategy || (appConfig.ui?.strategy as Strategy),
mergeWrapper ? { wrapper: $attrs?.class } : {},
$props.ui || {},
process.dev ? get(appConfig.ui, key, {}) : {},
toValue($config || {})
)
})
const attrs = computed(() => omit($attrs, ['class']))
const attrsClass = computed(() => mergeWrapper ? undefined : $attrs?.class as string)
return {
ui,
attrs,
attrsClass
}
}
So, useUI
should be used like this
const { ui, attrs, attrsClass } = useUI('button', props, config)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does makes sense, do you plan to open a PR to fix this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I have created a PR. You can view it here.
Resolves #703
Resolves #628
Resolves #669
Resolves #442
This PR will reduce the overall bundle size by only loading the config of the components you're actually using.
Props having a validator are now typed to have autocomplete:
app.config.ts
is now smartly merged with the default config usingtailwind-merge
(like theui
prop`)This will allow you (like explained in #509) to configure the components without rewriting everything:
The
button.color.white.solid
is'shadow-sm ring-1 ring-inset ring-gray-300 dark:ring-gray-700 text-gray-900 dark:text-white bg-white hover:bg-gray-50 disabled:bg-white dark:bg-gray-900 dark:hover:bg-gray-800/50 dark:disabled:bg-gray-900 focus-visible:ring-2 focus-visible:ring-primary-500 dark:focus-visible:ring-primary-400'
, doing so will only remove theshadow-sm
.The merging strategy defaults to
merge
but can be changed to its previous behaviouroverride
to avoid breaking changes.It is now also possible to configure the strategy per component instance using the
ui
prop:This will override the
solid
variant of the button completely instead of merging the classes.