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

DropdownMenu V2: add support for rendering in legacy popover slot #56342

Merged
merged 4 commits into from
Nov 23, 2023

Conversation

ciampo
Copy link
Contributor

@ciampo ciampo commented Nov 20, 2023

Part of #50459

What?

This PR introduces changes that allow the the new experimental version of DropdownMenu to render in the same slots as the legacy Popover-based components (including current stable DropdownMenu)

Why?

For better backwards compatibility between the new and the legacy version of the component

How?

  • The slot name can be passed either via a slotName prop, or via the popover slot name context
  • When a slot name isn't specified, the default popover slot name is used
  • If a slot associated to the computed slot name is found, the menu's popover is rendered in that slot. Otherwise the component behaves as the ariakit default.

Testing Instructions

  • Download the changes from this PR and run storybook locally
  • Visit the temporary Storybook examples that have been added to the new experimental DropdownMenu component. Open the dropdown menu and make sure that the menu is rendered correctly in the popover slot component (you can do so by inspecting the DOM and comparing it against the default story)
  • Compare the results from the point above to the equivalent Storybook examples for the legacy DropdownMenu component. The two components should behave the same way when picking the solt in which to render their popover.

@ciampo ciampo self-assigned this Nov 20, 2023
@ciampo ciampo requested review from a team and jsnajdr November 20, 2023 22:17
@ciampo ciampo added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components labels Nov 20, 2023
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes to Storybook will be removed before merging

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here, I'll undo these changes before merging

Copy link
Contributor

@chad1008 chad1008 left a comment

Choose a reason for hiding this comment

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

Looks good to me, and tests well based on your instructions 🚀 🚢

@ciampo ciampo enabled auto-merge (squash) November 23, 2023 11:58
@ciampo ciampo merged commit 0b6c4a0 into trunk Nov 23, 2023
50 checks passed
@ciampo ciampo deleted the feat/dropdown-menu-v2-ariakit-slotfill branch November 23, 2023 12:42
@github-actions github-actions bot added this to the Gutenberg 17.2 milestone Nov 23, 2023
@ciampo
Copy link
Contributor Author

ciampo commented Nov 23, 2023

I am going to revert this PR according to what discussed in #56482

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
Status: Done 🎉
Development

Successfully merging this pull request may close these issues.

2 participants