Skip to content

[ContextMenu][DropdownMenu] Integrate new submenu parts #629

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 11 commits into from
Jun 3, 2021

Conversation

andy-hook
Copy link
Contributor

@andy-hook andy-hook commented Apr 30, 2021

Builds on #627

Description

This PR uses the new submenu primitives from Menu to provide submenu support in ContextMenu and DropdownMenu

Approach

Menus can be nested to create sub menus.

Basic example

<ContextMenu>
  <ContextMenuTrigger/>
  <ContextMenuContent>
    <ContextMenuItem>Item 1</ContextMenuItem>
    <ContextMenuItem>Item 2</ContextMenuItem>
    
    <ContextMenu>
      <ContextMenuTrigger>
         Open Sub Menu
      </ContextMenuTrigger>

      <ContextMenuContent>
        <ContextMenuItem>Sub Item 1</ContextMenuItem>
        <ContextMenuItem>Sub Item 2</ContextMenuItem>
      </ContextMenuContent>
    </ContextMenu>
  </ContextMenuContent>
</ContextMenu>
submenu-mouse-kb.mov

@andy-hook andy-hook force-pushed the submenu-support branch 2 times, most recently from 861f8b6 to d04f58c Compare May 10, 2021 12:06
@andy-hook andy-hook closed this May 13, 2021
@andy-hook andy-hook force-pushed the submenu-support-context-dropdown branch from b06cdc7 to 732c76c Compare May 13, 2021 12:28
@andy-hook andy-hook reopened this May 13, 2021
@andy-hook andy-hook force-pushed the submenu-support-context-dropdown branch 2 times, most recently from 89c730c to 38b7c67 Compare May 14, 2021 14:49
@andy-hook andy-hook marked this pull request as ready for review May 14, 2021 14:52
@andy-hook andy-hook closed this May 21, 2021
@andy-hook andy-hook reopened this May 28, 2021
@andy-hook andy-hook marked this pull request as draft May 28, 2021 11:14
@jjenzz jjenzz force-pushed the submenu-support branch 2 times, most recently from 0f554f9 to 2b43c97 Compare May 28, 2021 14:57
@andy-hook andy-hook force-pushed the submenu-support-context-dropdown branch from 38b7c67 to e466b7c Compare May 28, 2021 15:26
@jjenzz jjenzz force-pushed the submenu-support branch from 2b43c97 to 4c07014 Compare May 31, 2021 18:17
@andy-hook andy-hook force-pushed the submenu-support-context-dropdown branch from 94b3a0c to 4e53fa9 Compare June 1, 2021 10:14
@andy-hook andy-hook marked this pull request as ready for review June 1, 2021 12:22
@andy-hook
Copy link
Contributor Author

@jjenzz @benoitgrelard – This one is also ready. Might be easier to review after we've worked through #627 though!

@jjenzz jjenzz force-pushed the submenu-support branch from fb8b3c5 to a4800b9 Compare June 1, 2021 17:08
Base automatically changed from submenu-support to submenu June 1, 2021 17:16
@jjenzz jjenzz force-pushed the submenu-support-context-dropdown branch from b326bd8 to a4f0f6f Compare June 1, 2021 17:24
Copy link
Contributor

@jjenzz jjenzz left a comment

Choose a reason for hiding this comment

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

Initial bits. Love how simple these changes were! Although, I'm a bit worried about the TS issues with the trigger parts, need to think about that some 😬

const DropdownMenu: React.FC<DropdownMenuOwnProps> = (props) => {
const parentSubmenuContext = React.useContext(SubmenuContext);
return (
<SubmenuContext.Provider value={parentSubmenuContext === undefined ? false : true}>
Copy link
Contributor

@jjenzz jjenzz Jun 3, 2021

Choose a reason for hiding this comment

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

In fact.... 💭

const SubmenuContext = React.createContext(false);
// ...
const isSubmenu = React.useContext(SubmenuContext);
<SubmenuContext.Provider value={isSubmenu}>

I remember we did this undefined check for ContextMenu because we needed to check if it was undefined to determine isRoot but we don't need that in DropdownMenu right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same applies here, we need undefined to determine the root, else it'll always fall one way or the other.

const DropdownMenu: React.FC<DropdownMenuOwnProps> = (props) => {
const parentMenuContext = React.useContext(SubmenuContext);
return (
<SubmenuContext.Provider value={parentMenuContext === undefined ? false : true}>
Copy link
Contributor

@jjenzz jjenzz Jun 3, 2021

Choose a reason for hiding this comment

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

I can't see a const isRoot = parentContext === undefined check anywhere in dropdown menu hence my confusion as to why we need parentMenuContext to be undefined. Can you help me understand this part?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the previous config we needed to have an additional state available for the first render to know if the context even exists, hence the undefined, but wrapping the content means we can do it after that point, so we implicitly know it's root. f9061af

@benoitgrelard benoitgrelard changed the title [ContextMenu][DropdownMenu] Add submenu support [ContextMenu][DropdownMenu] Expose submenu parts Jun 3, 2021
@benoitgrelard benoitgrelard changed the title [ContextMenu][DropdownMenu] Expose submenu parts [ContextMenu][DropdownMenu] Integrate new submenu parts Jun 3, 2021
@jjenzz jjenzz merged commit b27863f into submenu Jun 3, 2021
@jjenzz jjenzz deleted the submenu-support-context-dropdown branch June 3, 2021 15:44
benoitgrelard added a commit that referenced this pull request Jun 15, 2021
* [Menu] Add submenu primitives (#627)

Co-authored-by: Jenna Smith <[email protected]>

* [Menu] Provide predictable typeahead scoping (#662)

* Scope typeahead via collection

* Exclude only on typeahead space key

* Simplify typeahead

* Prevent continuous `updateSearch` call

* Inline `onExclusiveKeyDown` functionality

* Clear search buffer when leaving

* Move `MenuSubTrigger` code below `MenuContent`

* `isTypingAhead` conditional clarity

* [ContextMenu][DropdownMenu] Integrate new submenu parts (#629)

* [ContextMenu][DropdownMenu] Add `TriggerItem` part (#683)

* [ContextMenu][DropdownMenu] Don't close on sub trigger pointer down (#684)

Co-authored-by: Jenna Smith <[email protected]>

* [ContextMenu][DropdownMenu] Add pointer grace for navigation to sub-menus (#688)

* [ContextMenu][DropdownMenu] delay sub trigger open via pointer

* Add pointer area grace

* Use clientX/Y rather than pageX/Y because getBoundingClientRect doesn't include scroll

* Update DropdownMenu.stories.tsx

* feedback

* onItemEnter

* Update Menu.tsx

* more explicit open timer ref

* Remove content rect caching

* Move MenuItemImpl down (#689)

* [DropdownMenu] Add `aria-labelledby` to follow spec (#692)

* Add `aria-labelledby`

* Follow `aria-expanded` spec for submenu triggers

* Updates based on feedback

* Rename root context type

* [ContextMenu] Bring inline with `DropdownMenu` changes (#693)

* Remove `ArrowUp` to open (#702)

* [DropdownMenu] Enable `onEntryFocus` for root menu (#694)

* Focus first item when all content types open for keyboard users

* Initial feedback addressed

* Add `pointerdown` to keyboard user logic

* Remove capture

* Re-apply capture events with comment

* [ContextMenu][DropdownMenu] Improve pointer grace (#691)

* Share grace timer ref in content

* Align pointer behaviour closer with native

* Simplify base story for easier test

* Remove export of private func

* Tidy from feedback

* Minor naming adjustment

* Apply `useCallback`

Co-authored-by: Andy Hook <[email protected]>

* [ContextMenu][DropdownMenu] Fix menu focus after pointer interaction (#707)

* Fix menu focus after pointer interaction

* Add explainer

* [Menu] Remove `onPointerDownOutside` on sub (#708)

* [ContextMenu][DropdownMenu] Improve touch/mouse handling separation (#709)

* [ContextMenu][DropdownMenu] Improve touch/mouse handling separation

* Feedback

* [DropdownMenu][ContextMenu] Unwind all nested menus with ESC

* [Menu] Tidy `handleSelect` binding

* [Menu] Remove callbackRef in handleSelect

* [Menu] onSelect optional chain

Co-authored-by: Jenna Smith <[email protected]>
Co-authored-by: Benoît Grélard <[email protected]>
Co-authored-by: Jenna Smith <[email protected]>
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.

3 participants