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

Popover-based UI: do not render in Popover.Slot by default #56482

Open
2 of 7 tasks
ciampo opened this issue Nov 23, 2023 · 0 comments
Open
2 of 7 tasks

Popover-based UI: do not render in Popover.Slot by default #56482

ciampo opened this issue Nov 23, 2023 · 0 comments
Labels
[Package] Components /packages/components [Type] Enhancement A suggestion for improvement.

Comments

@ciampo
Copy link
Contributor

ciampo commented Nov 23, 2023

What problem does this address?

The subject of this conversation is influenced mainly by two related topics:

As we introduce new versions of existing components, one of the questions is whether these new components should try to tentatively render in Popover.Slot by default, like the internal Popover component currently does.

This conversation is also related (and a part of):

What is your proposed solution?

After discussing with @youknowriad , we established that we should always try to aim to offer the best possible experience to the consumers of our components — and we both agreed that the best experience is NOT to rely on Popover.Slot, and have our new components render their popovers in a new DOM node in the document.body.

We also discussed how in order to ensure a more coherent behavior between components using the internal Popover component and the new components using ariakit, we should also apply this new behavior to the first-party Popover component. This will likely require a few tweaks around Gutenberg (and a dev note), but we ultimately think that the change is going to result in an improvement of our components library.

Plan of action

  • Tweak the Popover component so that it doesn't render in Popover.Slot by default.
    • This should be likely achieved by removing the default value of the slotName prop.
    • We should check all usages in the editor for where it's effectively required for Popover to render in Popover.Slot (the block toolbar is definitely one such example)
    • We should check that there are not regressions introduced to how different popover-based UIs stack on top of each other
    • We should add a dev note, with an explanation of how to restore the previous behaviour (ie. passing an explicit slotName prop to Popover)
  • Make sure that all ariakit-based components do not render in Popover.Slot by default
    • Rever changes to the new version of DropdownMenu
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

No branches or pull requests

1 participant