feat(App): add global portal prop#3688
Conversation
commit: |
0d6145e to
11d2cdc
Compare
benjamincanac
left a comment
There was a problem hiding this comment.
I'm not sure I would put this config in global app.config but instead put it in the portal prop of each component as an object with the to and disabled fields. It would be more consistent and would allow configuration per component.
|
I've tried, and Implementing a system using a component-based configuration proved to be significantly more challenging than anticipated. The process required meticulous definition of the target for each individual component, leading to a highly time-consuming and cumbersome configuration process. This level of detail, while potentially offering great flexibility, introduces a substantial overhead. A potential solution to mitigate this complexity might involve a hybrid approach: employing a global configuration file to establish baseline settings, while allowing for component-specific overrides where necessary. This would reduce the repetitive nature of defining targets for every component, simplifying the overall configuration management. |
|
Then I would still not put it in App Config but as a prop on |
|
This is a great idea. |
2d6b07a to
566e49b
Compare
|
Is this the direction you had in mind? |
170b6fe to
9131e60
Compare
6c95b90 to
9959c22
Compare
|
Thanks @menthol for pushing this! Really helpful |
benjamincanac
left a comment
There was a problem hiding this comment.
Maybe we could create a usePortal composable to avoid code duplication that also exports its symbol like: https://github.com/nuxt/ui/blob/v3/src/runtime/composables/useAvatarGroup.ts#L4
WDYT?
You're welcome. I'm waiting for it to be reviewed to see if it's suitable. I hope it will be merged for 3.0.3 or 3.0.4. |
portalTarget supportportalTarget support
6102f3c to
97bedcf
Compare
b631f68 to
5db7a53
Compare
|
@benjamincanac What do you think of the changes? (i'm fixing the ci, but overall I think it's ready for review) |
benjamincanac
left a comment
There was a problem hiding this comment.
It looks good! Maybe we could improve it even further by returning target and to so you could do: const portalProps = usePortal(toRef(() => props.portal)) grouped with the rootProps, contentProps, etc. and v-bind="portalProps".
444b8ee to
851766d
Compare
|
@benjamincanac done |
c33199a to
533bcc4
Compare
Introduced `usePortal` composable to standardize portal handling across components, supporting `boolean`, `string`, and `HTMLElement` types. Updated various components to leverage this composable, improving configurability and integration with custom portal targets. Default behavior remains backward compatible.
portalTarget supportportal prop
|
@menthol I've cleanup up the code a bit, can you confirm that it's good to you? 😊 |
|
@benjamincanac |
|
Thanks @menthol! 😊 |
| const portalTarget = inject(portalTargetInjectionKey, undefined) | ||
|
|
||
| const to = computed(() => { | ||
| if (typeof portal.value === 'string' || portal.value instanceof HTMLElement) { |
There was a problem hiding this comment.
@menthol It seems instanceof HTMLElement breaks the build, can we check on typeof portal.value === 'boolean' instead? 🤔
There was a problem hiding this comment.
We can remove the HTMLElement check, was only for feature parity with Radix.
string and boolean work perfectly alone
There was a problem hiding this comment.
If the only problem is this instanceof, we can invert the condition
| const to = computed(() => { | ||
| if (typeof portal.value === 'string' || portal.value instanceof HTMLElement) { | ||
| return portal.value | ||
| } | ||
|
|
||
| return portalTarget?.value ?? 'body' | ||
| }) |
There was a problem hiding this comment.
import { inject, provide, computed, type Ref, type InjectionKey } from 'vue'
export const portalTargetInjectionKey: InjectionKey<Ref<string | HTMLElement>> = Symbol('nuxt-ui.portal-target')
export function usePortal(portal: Ref<string | HTMLElement | boolean | undefined>) {
const portalTarget = inject(portalTargetInjectionKey, undefined)
const to = computed(() => {
if (typeof portal.value === 'boolean' || portal.value === undefined) {
return portalTarget?.value ?? 'body'
}
return portal.value
})
const disabled = computed(() => typeof portal.value === 'boolean' ? !portal.value : false)
provide(portalTargetInjectionKey, computed(() => to.value))
return computed(() => ({
to: to.value,
disabled: disabled.value
}))
}
🔗 Linked issue
Resolves #3687
❓ Type of change
📚 Description
Introduced a
portalTargetoption inUAppto specify portal mounting targets across various components. Updated portals in components likeContextMenu,Drawer,Tooltip, and others to respectportalTarget. Default target is set tobody.📝 Checklist