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

Submenu support #4976

Merged
merged 168 commits into from Nov 30, 2023
Merged

Submenu support #4976

merged 168 commits into from Nov 30, 2023

Conversation

LFDanLu
Copy link
Member

@LFDanLu LFDanLu commented Aug 25, 2023

Closes #1356, https://github.com/orgs/adobe/projects/19/views/20?pane=issue&itemId=35511606, https://github.com/orgs/adobe/projects/19/views/20?pane=issue&itemId=35511675, https://github.com/orgs/adobe/projects/19/views/20?pane=issue&itemId=43823551, https://github.com/orgs/adobe/projects/19/views/20?pane=issue&itemId=43393200, https://github.com/orgs/adobe/projects/19/views/20?pane=issue&itemId=45328738

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

RSP

using existing ContextualHelpTrigger and ListBox to see what is needed for the sub menu
…tion

SubMenuTrigger hasnt been tested, just grabs stuff from MenuTrigger and ContextualHelpTrigger that I deemed suitable
add support for opening submenu via Enter/Space/ArrowRight and closing submenu via ArrowLeft. Also adds the proper autoFocus behavior based on the interaction that opened the submenu
…ailable=false

user provided onClose wasnt firing because we were always sourcing it from the MenuTriggers context onClose. ContextualHelpTrigger also needed additional logic to not render a sub menu if isUnavailable is false
… whole trigger state to useMenu

this makes the api a bit nicer
current approach has the SubMenuTrigger adding a prop to the Item let selectionManager know that it should be non-selectable. Other approaches would involve additional changes to useSelectableItem to disable press props but keep keyboard navigation
…us a private prop

dont really feel like users will need to pass in something for onCloseAllMenus so made it a hidden prop. Also moved it into useMenu so it doesnt matter if the user is focused on the menu or a menu item when hitting Esc
also made onSubMenuClose a private prop
@LFDanLu LFDanLu changed the title (WIP Submenu support (WIP) Submenu support Aug 25, 2023
@rspbot
Copy link

rspbot commented Nov 20, 2023

updates propName to type as per review, fixes VO dismiss button with contextual help and reintroduces the transitions back to submenu since there were some weird transforms that cause visible jerks in position when closing the submenus without the visibility transition.
@rspbot
Copy link

rspbot commented Nov 21, 2023

@rspbot
Copy link

rspbot commented Nov 21, 2023

instead of informing selection manager directly that the submenu item is a non selectable item, opt for change in useMenuItem instead since it feels a bit too specific to include in selection manager. Still need onFocus though so we still update the tracked key when user clicks on a submenu trigger in a Tray
@rspbot
Copy link

rspbot commented Nov 21, 2023

removed the transform for submenus and replaced it with a offset calculated for Popover instead so that we dont get the weird jerk animation when the submenu unmounts
…/closing the menu with keyboard

if the user presses left/right rapidly enough, the submenu doesnt actually unmount due to being in the middle of a transition animation, meaning focus wasnt being handled by useSelectableCollection focus useEffect which only run on mount. This fixes that by running focus and openSubmenu in those cases
@rspbot
Copy link

rspbot commented Nov 27, 2023

@rspbot
Copy link

rspbot commented Nov 28, 2023

@rspbot
Copy link

rspbot commented Nov 28, 2023

Copy link
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

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

looks pretty good! great work to both of you

@rspbot
Copy link

rspbot commented Nov 28, 2023

@rspbot
Copy link

rspbot commented Nov 30, 2023

@rspbot
Copy link

rspbot commented Nov 30, 2023

## API Changes

unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }

@react-aria/menu

AriaMenuItemProps

 AriaMenuItemProps {
+  aria-controls?: string
+  aria-expanded?: boolean | 'true' | 'false'
   aria-haspopup?: 'menu' | 'dialog'
   aria-label?: string
   closeOnSelect?: boolean = true
   isVirtualized?: boolean
 }
 

it changed:

  • useMenuItem

UNSTABLE_useSubmenuTrigger

changed by:

  • AriaSubmenuTriggerProps
  • SubmenuTriggerAria
  • SubmenuTriggerProps
-
+UNSTABLE_useSubmenuTrigger<T> {
+  props: AriaSubmenuTriggerProps
+  state: SubmenuTriggerState
+  ref: RefObject<FocusableElement>
+  returnVal: undefined
+}

AriaSubmenuTriggerProps

-
+AriaSubmenuTriggerProps {
+  isDisabled?: boolean
+  node: RSNode<unknown>
+  parentMenuRef: RefObject<HTMLElement>
+  submenuRef: RefObject<HTMLElement>
+  type?: 'dialog' | 'menu'
+}

it changed:

  • UNSTABLE_useSubmenuTrigger

SubmenuTriggerAria

changed by:

  • SubmenuTriggerProps
-
+SubmenuTriggerAria<T> {
+  popoverProps: (Pick<AriaPopoverProps, 'isNonModal' | 'shouldCloseOnInteractOutside'> & Pick<OverlayProps, 'disableFocusManagement'>)
+  submenuProps: SubmenuProps<T>
+  submenuTriggerProps: SubmenuTriggerProps
+}

it changed:

  • UNSTABLE_useSubmenuTrigger

@react-aria/overlays

AriaPopoverProps

 AriaPopoverProps {
   isKeyboardDismissDisabled?: boolean = false
   isNonModal?: boolean
   popoverRef: RefObject<Element>
+  shouldCloseOnInteractOutside?: (Element) => boolean
   triggerRef: RefObject<Element>
 }

it changed:

  • usePopover

@react-spectrum/menu

SpectrumMenuProps

-
+SubmenuTrigger {
+  children: Array<ReactElement>
+  targetKey: Key
+}

SpectrumMenuTriggerProps

-
+SpectrumSubmenuTriggerProps {
+  children: Array<ReactElement>
+}

@react-spectrum/overlays

Overlay

 Popover {
   children: ReactNode
   container?: HTMLElement
   disableFocusManagement?: boolean
   enableBothDismissButtons?: boolean
   hideArrow?: boolean
+  onDismissButtonPress?: () => void
   onEnter?: () => void
   onEntered?: () => void
   onEntering?: () => void
   onExit?: () => void
   onExiting?: () => void
   shouldContainFocus?: boolean
   state: OverlayTriggerState
 }
 

@react-stately/menu

MenuTriggerState

 MenuTriggerState {
+  UNSTABLE_closeSubmenu: (Key, number) => void
+  UNSTABLE_expandedKeysStack: Array<Key>
+  UNSTABLE_openSubmenu: (Key, number) => void
+  close: () => void
   focusStrategy: FocusStrategy
   open: (FocusStrategy | null) => void
   toggle: (FocusStrategy | null) => void
 }

it changed:

  • useMenuTriggerState

UNSTABLE_useSubmenuTriggerState

changed by:

  • SubmenuTriggerProps
  • SubmenuTriggerState
-
+UNSTABLE_useSubmenuTriggerState {
+  props: SubmenuTriggerProps
+  state: MenuTriggerState
+  returnVal: undefined
+}

SubmenuTriggerProps

-
+SubmenuTriggerProps {
+  triggerKey: Key
+}

it changed:

  • SubmenuTriggerAria
  • UNSTABLE_useSubmenuTrigger
  • UNSTABLE_useSubmenuTriggerState

SubmenuTriggerState

-
+SubmenuTriggerState {
+  close: () => void
+  closeAll: () => void
+  focusStrategy: FocusStrategy | null
+  isOpen: boolean
+  open: (FocusStrategy | null) => void
+  submenuLevel: number
+  toggle: (FocusStrategy | null) => void
+}

it changed:

  • UNSTABLE_useSubmenuTriggerState

@LFDanLu LFDanLu merged commit a2146aa into main Nov 30, 2023
26 checks passed
@LFDanLu LFDanLu deleted the submenu_alt_api branch November 30, 2023 23:21
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.

Is there nested menu support?
6 participants