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

feat[Dropdown Menu]: Nested menu item is a mf #87

Merged
merged 25 commits into from Jul 10, 2023
Merged

Conversation

khairulhaaziq
Copy link
Member

@khairulhaaziq khairulhaaziq commented Jun 27, 2023

Intended behaviors:

Submenu must work perfectly regardless number of child submenus(recursively)

Mouse:

  • Subtrigger: Hover will open submenu while still selecting the trigger
  • Hover any items will close every child menu

Keyboard:

  • Subtrigger: Keydown ArrowRight will trigger submenu to open and select the first item on the submenu

Resource:
https://codesandbox.io/s/admiring-lamport-5wt3yg?file=/src/DropdownMenu.tsx

@khairulhaaziq khairulhaaziq mentioned this pull request Jun 28, 2023
9 tasks
@zernonia zernonia self-requested a review July 10, 2023 09:06
@zernonia zernonia marked this pull request as ready for review July 10, 2023 09:06
@khairulhaaziq
Copy link
Member Author

I think overall the dropdown works great currently, havent tested really heavily but so far so good. Tested and some things are broken:

  1. Dropdown menu collision aware isnt working:
Screenshot 2023-07-10 at 6 07 10 PM
  1. Hovercard arrow is misplaced when moved position do to collision:
Screenshot 2023-07-10 at 6 07 01 PM

@khairulhaaziq
Copy link
Member Author

khairulhaaziq commented Jul 10, 2023

to fix the arrow, this is what I think
the previous thing I did to pass the placement applied is by passing via refs instead of static value, because the placement is changed on nexttick instead of when rendered, so what Im thinking this placedSide should pass a Ref instead of Side.

// PopperContent.vue
177 const [placedSide, placedAlign] = getSideAndAlignFromPlacement(placement.value);

@zernonia
Copy link
Collaborator

Pushed a fix @khairulhaaziq . It should respect the collision now 😁

@khairulhaaziq
Copy link
Member Author

khairulhaaziq commented Jul 10, 2023

Perfect! Lgtm

Screenshot 2023-07-10 at 8 12 57 PM

@zernonia you can review and then merge if its okay

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

Successfully merging this pull request may close these issues.

None yet

2 participants