Skip to content

feat(App): add global portal prop #3688

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

Merged
merged 4 commits into from
Apr 21, 2025
Merged

feat(App): add global portal prop #3688

merged 4 commits into from
Apr 21, 2025

Conversation

menthol
Copy link
Contributor

@menthol menthol commented Mar 26, 2025

πŸ”— Linked issue

Resolves #3687

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality)
  • ✨ 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

Introduced a portalTarget option in UApp to specify portal mounting targets across various components. Updated portals in components like ContextMenu, Drawer, Tooltip, and others to respect portalTarget. Default target is set to body.

πŸ“ Checklist

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

Copy link

pkg-pr-new bot commented Mar 26, 2025

npm i https://pkg.pr.new/@nuxt/ui@3688

commit: c4807e0

@menthol menthol force-pushed the v3 branch 2 times, most recently from 0d6145e to 11d2cdc Compare March 26, 2025 07:15
Copy link
Member

@benjamincanac benjamincanac left a comment

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

menthol commented Mar 26, 2025

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.

Copy link
Member

Then I would still not put it in App Config but as a prop on UApp that is provided to all components maybe?

@menthol
Copy link
Contributor Author

menthol commented Mar 26, 2025

This is a great idea.

@menthol menthol force-pushed the v3 branch 4 times, most recently from 2d6b07a to 566e49b Compare March 26, 2025 19:27
@menthol
Copy link
Contributor Author

menthol commented Mar 26, 2025

Is this the direction you had in mind?

@menthol menthol requested a review from benjamincanac March 27, 2025 15:38
@menthol menthol force-pushed the v3 branch 5 times, most recently from 170b6fe to 9131e60 Compare March 31, 2025 18:02
@benjamincanac benjamincanac added the v3 #1289 label Apr 4, 2025
@menthol menthol force-pushed the v3 branch 2 times, most recently from 6c95b90 to 9959c22 Compare April 15, 2025 17:42
@sebastiannoell
Copy link

Thanks @menthol for pushing this! Really helpful

Copy link
Member

@benjamincanac benjamincanac left a comment

Choose a reason for hiding this comment

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

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?

@menthol
Copy link
Contributor Author

menthol commented Apr 16, 2025

Thanks @menthol for pushing this! Really helpful

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.

@benjamincanac benjamincanac changed the title feat(portal): add configurable portalTarget support feat(App): add configurable portalTarget support Apr 16, 2025
@menthol menthol force-pushed the v3 branch 4 times, most recently from 6102f3c to 97bedcf Compare April 16, 2025 15:45
@menthol menthol force-pushed the v3 branch 3 times, most recently from b631f68 to 5db7a53 Compare April 16, 2025 15:51
@menthol
Copy link
Contributor Author

menthol commented Apr 16, 2025

@benjamincanac What do you think of the changes? (i'm fixing the ci, but overall I think it's ready for review)

Copy link
Member

@benjamincanac benjamincanac left a comment

Choose a reason for hiding this comment

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

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".

@menthol menthol force-pushed the v3 branch 3 times, most recently from 444b8ee to 851766d Compare April 16, 2025 16:31
@menthol
Copy link
Contributor Author

menthol commented Apr 16, 2025

@benjamincanac done

@menthol menthol force-pushed the v3 branch 2 times, most recently from c33199a to 533bcc4 Compare April 16, 2025 16:55
@menthol menthol requested a review from benjamincanac April 17, 2025 09:59
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.
@benjamincanac benjamincanac changed the title feat(App): add configurable portalTarget support feat(App): add global portal prop Apr 21, 2025
@benjamincanac
Copy link
Member

@menthol I've cleanup up the code a bit, can you confirm that it's good to you? 😊

@menthol
Copy link
Contributor Author

menthol commented Apr 21, 2025

@benjamincanac
LGTM,
but removing the computed in usePortal, you make the return not reactive anymore

@benjamincanac benjamincanac merged commit 29fa462 into nuxt:v3 Apr 21, 2025
8 of 9 checks passed
@benjamincanac
Copy link
Member

Thanks @menthol! 😊

const portalTarget = inject(portalTargetInjectionKey, undefined)

const to = computed(() => {
if (typeof portal.value === 'string' || portal.value instanceof HTMLElement) {
Copy link
Member

Choose a reason for hiding this comment

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

@menthol It seems instanceof HTMLElement breaks the build, can we check on typeof portal.value === 'boolean' instead? πŸ€”

Copy link
Member

Choose a reason for hiding this comment

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

CleanShot 2025-04-22 at 10 54 31@2x

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can remove the HTMLElement check, was only for feature parity with Radix.

string and boolean work perfectly alone

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the only problem is this instanceof, we can invert the condition

Comment on lines +8 to +14
const to = computed(() => {
if (typeof portal.value === 'string' || portal.value instanceof HTMLElement) {
return portal.value
}

return portalTarget?.value ?? 'body'
})
Copy link
Contributor Author

@menthol menthol Apr 22, 2025

Choose a reason for hiding this comment

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

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
  }))
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New PR: #3954

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v3 #1289
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow specify portal destination for all portaled components
3 participants