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

Feature : Refactor Autocomplete using DropdownMenu #66192

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

Vrishabhsk
Copy link
Contributor

What?

  • Refactor Autocomplete component using DropdownMenu instead of custom ListBox

Why?

Issues

  • The Type / to chose a block text disappears when the / is erased
  • After typing /, it works perfectly but when the popover is open, and we switch tab or the window and return to editor and remove the /, an error is thrown which breaks the block.

@Vrishabhsk Vrishabhsk requested a review from ajitbohra as a code owner October 17, 2024 09:09
Copy link

github-actions bot commented Oct 17, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: Vrishabhsk <[email protected]>
Co-authored-by: ciampo <[email protected]>
Co-authored-by: mirka <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@Vrishabhsk
Copy link
Contributor Author

Hi @mirka 👋

  • I have refactored the Autocomplete component as per this issue : #64323
  • There are some issue which I need guidance upon mentioned in the PR description.
  • Due to the blockers, this isn't the final version of the implementation, ill be updating the docs and tests once the feature looks good to me.

I was wondering if you could help me with resolving these blockers? Thanks

@mirka mirka added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components labels Oct 17, 2024
@mirka mirka requested a review from a team October 17, 2024 10:27
@ciampo
Copy link
Contributor

ciampo commented Oct 18, 2024

Hey @Vrishabhsk , thank you for the work so far!

Before diving into detailed code review, I have some high-level feedback:

  • the original issue called for using DropdownMenuV2, if possible — you're using DropdownMenu instead (sorry if that part was confusing!)
  • regardless, I'm not sure we can use the DropdownMenu (or DropdownMenuV2) component in their current state, since they both render a visual trigger, while (from what I understand) Autocomplete doesn't currently come with a trigger button (in the editor, in fact, it can be triggered by pressing the / shortcut)

Having said that, if we tweak DropdownMenuV2 to expose the "menu in a popover" part separately from the "trigger" part, we may be able to actually rewrite `Autocomplete.

Alternatively, we could simply deprecate Autocomplete and refactor its usages in the editor with said DropdownMenuV2 subcomponents.

Does this make sense, @mirka , or am I misinterpreting the task?

@Vrishabhsk
Copy link
Contributor Author

Hi @ciampo 👋

  • Initially, I had used DropdownMenuV2 but due to several issues which I encountered, I instead used DropdownMenu
  • The visual trigger in this case has been handled by defaultOpen and open attribute which keeps the Dropdown visible at all times.
  • The dropdown will be visible only when the Autocomplete component is rendered
  • The icon attribute in DropdownMenu has been set to <></> which prevent the trigger from appearing
  • You can view the implementation via DropdownMenuV2 : DropdownMenuV2 Refactor

Let me know the further steps here. Thanks

@ciampo
Copy link
Contributor

ciampo commented Oct 24, 2024

I'd like to hear @mirka 's thoughts too before deciding next steps.

In any case, if we were to use a dropdown menu implementation, I think it should be the V2 (which, by the way, is still experimental and is currently undergoing major API changes like #66289 and #66383, which should let us avoid hacks like rendering empty fragments for the icon trigger etc).

I had a quick look at your refactor and could spot a few parts that need changing. More in general, I believe that using the dropdown menu component may require a bit of a deeper refactor than simply swapping in components.

@mirka
Copy link
Member

mirka commented Oct 25, 2024

if we tweak DropdownMenuV2 to expose the "menu in a popover" part separately from the "trigger" part

@ciampo This sounds good to me. I remember at some point we were talking about replacing the trigger prop with a subcomponent anyway.

In this case, next steps would be to make changes in DropdownMenuV2?

@ciampo
Copy link
Contributor

ciampo commented Oct 25, 2024

I remember at some point we were talking about replacing the trigger prop with a subcomponent anyway.

In this case, next steps would be to make changes in DropdownMenuV2?

Yup! Working on these changes in #66383, will ping you all once they're ready for review

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

Successfully merging this pull request may close these issues.

3 participants