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

Improve menu controlling with onOpenChange #17712

Merged
merged 8 commits into from Apr 6, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 8 additions & 3 deletions packages/react-examples/src/react-menu/Menu/Menu.stories.tsx
Expand Up @@ -33,10 +33,15 @@ export const DefaultOpen = () => <TextOnly defaultOpen />;

export const ControlledPopup = () => {
const [open, setOpen] = React.useState(false);
const onOpenChange: MenuProps['onOpenChange'] = (e, data) => {
console.log('xxxx');
ling1726 marked this conversation as resolved.
Show resolved Hide resolved
setOpen(data.open);
};

return (
<Menu open={open}>
<Menu open={open} onOpenChange={onOpenChange}>
<MenuTrigger>
<button onClick={() => setOpen(s => !s)}>Toggle menu</button>
<button>Toggle menu</button>
</MenuTrigger>

<MenuList>
Expand All @@ -57,7 +62,7 @@ export const MenuTriggerInteractions = () => {
};

export const NestedSubmenus = () => (
<Menu>
<Menu onOpenChange={open => console.log('open', open)}>
<MenuTrigger>
<button>Toggle menu</button>
</MenuTrigger>
Expand Down
7 changes: 6 additions & 1 deletion packages/react-menu/etc/react-menu.api.md
Expand Up @@ -193,6 +193,7 @@ export interface MenuProps extends MenuListProps {
onContext?: boolean;
// (undocumented)
onHover?: boolean;
onOpenChange?: (e: React.SyntheticEvent<HTMLElement>, data: OnOpenChangeData) => void;
open?: boolean;
position?: PositioningProps['position'];
}
Expand All @@ -212,7 +213,7 @@ export interface MenuState extends MenuProps {
menuTrigger: React.ReactNode;
open: boolean;
ref: React.MutableRefObject<HTMLElement>;
setOpen: React.Dispatch<React.SetStateAction<boolean>>;
setOpen: (e: React.SyntheticEvent<HTMLElement>, open: boolean) => void;
triggerId: string;
triggerRef: React.MutableRefObject<HTMLElement>;
}
Expand All @@ -236,6 +237,10 @@ export interface MenuTriggerState extends MenuTriggerProps {
ref: React.MutableRefObject<HTMLElement>;
}

// @public
export interface OnOpenChangeData extends Pick<MenuState, 'open'> {
}

// @public
export const renderMenu: (state: MenuState) => JSX.Element;

Expand Down
13 changes: 12 additions & 1 deletion packages/react-menu/src/components/Menu/Menu.types.ts
Expand Up @@ -18,6 +18,12 @@ export interface MenuProps extends MenuListProps {
*/
open?: boolean;

/**
* Call back when the component requests to change value
* The `open` value is used as a hint when directly controlling the component
*/
onOpenChange?: (e: React.SyntheticEvent<HTMLElement>, data: OnOpenChangeData) => void;

/**
* Whether the popup is open by default
*/
Expand Down Expand Up @@ -66,7 +72,7 @@ export interface MenuState extends MenuProps {
/**
* Callback to open/close the popup
*/
setOpen: React.Dispatch<React.SetStateAction<boolean>>;
setOpen: (e: React.SyntheticEvent<HTMLElement>, open: boolean) => void;

/**
* Internal react node that just simplifies handling children
Expand Down Expand Up @@ -103,3 +109,8 @@ export interface MenuState extends MenuProps {
*/
isSubmenu: boolean;
}

/**
* Data attached to open/close events
*/
export interface OnOpenChangeData extends Pick<MenuState, 'open'> {}
17 changes: 13 additions & 4 deletions packages/react-menu/src/components/Menu/useMenu.tsx
Expand Up @@ -78,19 +78,28 @@ export const useMenu = (props: MenuProps, ref: React.Ref<HTMLElement>, defaultPr
const [open, setOpen] = useControllableValue(state.open, state.defaultOpen);
// TODO fix useControllableValue typing
state.open = open !== undefined ? open : state.open;
const { onOpenChange } = state;
state.setOpen = React.useCallback(
(...args) => {
setOpen(...args);
(e, shouldOpen) => {
setOpen(prevOpen => {
// More than one event (mouse, focus, keyboard) can request the popup to close
// We assume the first event is the correct one
if (prevOpen !== shouldOpen) {
onOpenChange?.(e, { open: shouldOpen });
}

return shouldOpen;
});
},
[setOpen],
[setOpen, onOpenChange],
ling1726 marked this conversation as resolved.
Show resolved Hide resolved
);

useMenuSelectableState(state);
useMenuPopup(state);
useOnClickOutside({
element: document,
refs: [state.menuPopupRef, triggerRef],
callback: () => setOpen(false),
callback: e => state.setOpen((e as unknown) as React.MouseEvent<HTMLElement>, false),
ling1726 marked this conversation as resolved.
Show resolved Hide resolved
});

return state;
Expand Down
8 changes: 4 additions & 4 deletions packages/react-menu/src/components/Menu/useMenuPopup.ts
Expand Up @@ -38,23 +38,23 @@ export const useMenuPopup = (state: UseMenuPopupState) => {

newProps.onMouseEnter = (e: React.MouseEvent<HTMLElement>) => {
if (onHover && !onContext) {
setOpen(true);
setOpen(e, true);
}

originalProps?.onMouseEnter?.(e);
};

newProps.onBlur = (e: React.FocusEvent<HTMLElement>) => {
if (isOutsideMenu({ triggerRef, menuPopupRef, event: e })) {
setOpen(false);
setOpen(e, false);
}
originalProps?.onBlur?.(e);
};

newProps.onKeyDown = (e: React.KeyboardEvent<HTMLElement>) => {
const keyCode = getCode(e);
if (keyCode === keyboardKey.Escape || (isSubmenu && keyCode === keyboardKey.ArrowLeft)) {
setOpen(false);
setOpen(e, false);
dismissedWithKeyboardRef.current = true;
e.stopPropagation(); // Left and Escape should only close one menu at a time
}
Expand All @@ -72,7 +72,7 @@ export const useMenuPopup = (state: UseMenuPopupState) => {
// Assumption made that all clicks will close the popup if propagated from children
// To stop clicks from closing the menu call stopPropagation
newProps.onClick = (e: React.MouseEvent<HTMLElement>) => {
setOpen(false);
setOpen(e, false);
originalProps?.onClick?.(e);
};

Expand Down
Expand Up @@ -56,7 +56,7 @@ describe('useTriggerElement', () => {

// Assert
expect(spy).toHaveBeenCalledTimes(1);
expect(spy).toHaveBeenCalledWith(true);
expect(spy).toHaveBeenCalledWith(expect.anything(), true);
});

it('should not open menu if child is disabled', () => {
Expand Down Expand Up @@ -98,7 +98,7 @@ describe('useTriggerElement', () => {

// Assert
expect(spy).toHaveBeenCalledTimes(1);
expect(spy).toHaveBeenCalledWith(expectedValue);
expect(spy).toHaveBeenCalledWith(expect.anything(), expectedValue);
});

it.each([
Expand Down Expand Up @@ -157,7 +157,7 @@ describe('useTriggerElement', () => {

// Assert
expect(spy).toHaveBeenCalledTimes(1);
expect(spy).toHaveBeenCalledWith(true);
expect(spy).toHaveBeenCalledWith(expect.anything(), true);
});

it('should not open menu if child is disabled', () => {
Expand Down Expand Up @@ -204,7 +204,7 @@ describe('useTriggerElement', () => {

// Assert
expect(spy).toHaveBeenCalledTimes(1);
expect(spy).toHaveBeenCalledWith(true);
expect(spy).toHaveBeenCalledWith(expect.anything(), true);
});

it('should open menu on down arrow for root trigger', () => {
Expand All @@ -220,6 +220,6 @@ describe('useTriggerElement', () => {

// Assert
expect(spy).toHaveBeenCalledTimes(1);
expect(spy).toHaveBeenCalledWith(true);
expect(spy).toHaveBeenCalledWith(expect.anything(), true);
});
});
Expand Up @@ -39,28 +39,28 @@ export const useTriggerElement = (state: UseTriggerElementState): MenuTriggerSta
// TODO also need to warn on React.Fragment usage
const child = React.Children.only(state.children);

const onContextMenu = useEventCallback((e: React.MouseEvent) => {
const onContextMenu = useEventCallback((e: React.MouseEvent<HTMLElement>) => {
if (onContext) {
e.preventDefault();
setOpen(true);
setOpen(e, true);
}
child.props?.onContextMenu?.(e);
});

const onClick = useEventCallback((e: React.MouseEvent) => {
const onClick = useEventCallback((e: React.MouseEvent<HTMLElement>) => {
// Click event will close the menu popup
// Therefore, do not propagate click events to parent popup for nested menu trigger
if (isSubmenu) {
e.stopPropagation();
}

if (!onContext) {
setOpen(!open);
setOpen(e, !open);
}
child.props?.onClick?.(e);
});

const onKeyDown = useEventCallback((e: React.KeyboardEvent) => {
const onKeyDown = useEventCallback((e: React.KeyboardEvent<HTMLElement>) => {
if (shouldPreventDefaultOnKeyDown(e)) {
e.preventDefault();
openedWithKeyboardRef.current = true;
Expand All @@ -73,23 +73,23 @@ export const useTriggerElement = (state: UseTriggerElementState): MenuTriggerSta
((isSubmenu && keyCode === keyboardKey.ArrowRight) || (!isSubmenu && keyCode === keyboardKey.ArrowDown))
) {
openedWithKeyboardRef.current = true;
setOpen(true);
setOpen(e, true);
}

child.props?.onKeyDown?.(e);
});

const onMouseEnter = useEventCallback((e: React.MouseEvent) => {
const onMouseEnter = useEventCallback((e: React.MouseEvent<HTMLElement>) => {
if (onHover && !onContext) {
setOpen(true);
setOpen(e, true);
}
child.props?.onMouseEnter?.(e);
});

// no mouse leave, since mouse enter sets focus for menu items
const onBlur = useEventCallback((e: React.FocusEvent) => {
const onBlur = useEventCallback((e: React.FocusEvent<HTMLElement>) => {
if (isOutsideMenu({ menuPopupRef, triggerRef, event: e })) {
setOpen(false);
setOpen(e, false);
}

child.props?.onBlur?.(e);
Expand Down