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.

Sign up for GitHub

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

Jump to bottom

[MenuUnstyled] Accept callbacks in componentsProps #32997

Merged
merged 7 commits into from
Jun 10, 2022
Merged

[MenuUnstyled] Accept callbacks in componentsProps #32997

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e19c84a
Accept function in Menu's componentsProps
michaldudak Jun 2, 2022
8757572
Accept functions in MenuItemUnstyled's componentsProps
michaldudak Jun 2, 2022
40bc050
Create and use the useSlotProps hook
michaldudak Jun 6, 2022
c971a38
Merge remote-tracking branch 'upstream/master' into componentsProps/menu
michaldudak Jun 7, 2022
e87d050
Merge remote-tracking branch 'upstream/master' into componentsProps/menu
michaldudak Jun 8, 2022
3efae41
Add useSlotProps tests
michaldudak Jun 9, 2022
a24c5b2
Merge remote-tracking branch 'upstream/master' into componentsProps/menu
michaldudak Jun 9, 2022
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
1 change: 0 additions & 1 deletion packages/mui-base/src/MenuItemUnstyled/MenuItemUnstyled.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ describe('MenuItemUnstyled', () => {
},
skip: [
'reactTestRenderer', // Need to be wrapped in MenuUnstyledContext
'componentsPropsCallbacks', // not implemented yet
],
}));
});
17 changes: 9 additions & 8 deletions packages/mui-base/src/MenuItemUnstyled/MenuItemUnstyled.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
import * as React from 'react';
import PropTypes from 'prop-types';
import clsx from 'clsx';
import { MenuItemOwnerState, MenuItemUnstyledProps } from './MenuItemUnstyled.types';
import { MenuItemUnstyledOwnerState, MenuItemUnstyledProps } from './MenuItemUnstyled.types';
import { appendOwnerState } from '../utils';
import { getMenuItemUnstyledUtilityClass } from './menuItemUnstyledClasses';
import useMenuItem from './useMenuItem';
import composeClasses from '../composeClasses';
import resolveComponentProps from '../utils/resolveComponentProps';

function getUtilityClasses(ownerState: MenuItemOwnerState) {
function getUtilityClasses(ownerState: MenuItemUnstyledOwnerState) {
const { disabled, focusVisible } = ownerState;

const slots = {
Expand Down Expand Up @@ -42,25 +43,25 @@ const MenuItemUnstyled = React.forwardRef(function MenuItemUnstyled(
...other
} = props;

const Root = component ?? components.Root ?? 'li';

const { getRootProps, disabled, focusVisible } = useMenuItem({
disabled: disabledProp,
ref,
label,
});

const ownerState: MenuItemOwnerState = { ...props, disabled, focusVisible };
const ownerState: MenuItemUnstyledOwnerState = { ...props, disabled, focusVisible };

const classes = getUtilityClasses(ownerState);

const Root = component ?? components.Root ?? 'li';
const rootComponentProps = resolveComponentProps(componentsProps.root, ownerState);
const rootProps = appendOwnerState(
Root,
{
...other,
...componentsProps.root,
...getRootProps(other),
className: clsx(classes.root, className, componentsProps.root?.className),
...rootComponentProps,
Copy link
Member

Choose a reason for hiding this comment

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

hmm why was the order changed? Shouldn't the getRootProps() comes last as they are adding some event handlers?

Copy link
Member Author

Choose a reason for hiding this comment

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

This way devs are able to override whatever the hook is setting. But I see there's a problem with event handlers. I suppose the order should be as following:

  1. Props from the hook (with event handlers passed in to be chained)
  2. Additional props from the component (...other) with removed event handlers - only for the root slot
  3. componentsProps.<slot name> with removed event handlers
  4. className - merged from className prop, componentsProps.*.className and slot-specific classes
  5. ref - if necessary

Developers would be able to override whatever prop they need and if they provide an event handler, it would be chained with the hook's handlers.

We could use a function that would encapsulate this logic and ensure this works consistently across our components.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Yep, let's do this 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

I created a useSlotProps function that takes care of everything needed to resolve, merge and supplement the props with ownerState. The preparation of the slots' props in the components is much cleaner now.

className: clsx(classes.root, className, rootComponentProps?.className),
},
ownerState,
);
Expand Down Expand Up @@ -95,7 +96,7 @@ MenuItemUnstyled.propTypes /* remove-proptypes */ = {
* @ignore
*/
componentsProps: PropTypes.shape({
root: PropTypes.object,
root: PropTypes.oneOfType([PropTypes.func, PropTypes.object]),
}),
/**
* If `true`, the menu item will be disabled.
Expand Down
9 changes: 7 additions & 2 deletions packages/mui-base/src/MenuItemUnstyled/MenuItemUnstyled.types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import * as React from 'react';
import { SlotComponentProps } from '../utils';

export interface MenuItemUnstyledComponentsPropsOverrides {}

export interface MenuItemOwnerState extends MenuItemUnstyledProps {
export interface MenuItemUnstyledOwnerState extends MenuItemUnstyledProps {
disabled: boolean;
focusVisible: boolean;
}
Expand All @@ -21,7 +22,11 @@ export interface MenuItemUnstyledProps {
Root?: React.ElementType;
};
componentsProps?: {
root?: React.ComponentPropsWithRef<'li'> & MenuItemUnstyledComponentsPropsOverrides;
root?: SlotComponentProps<
'li',
MenuItemUnstyledComponentsPropsOverrides,
MenuItemUnstyledOwnerState
>;
};
/**
* A text representation of the menu item's content.
Expand Down
8 changes: 1 addition & 7 deletions packages/mui-base/src/MenuUnstyled/MenuUnstyled.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,7 @@ describe('MenuUnstyled', () => {
expectedClassName: menuUnstyledClasses.listbox,
},
},
skip: [
'reactTestRenderer',
'propsSpread',
'componentProp',
'componentsProp',
'componentsPropsCallbacks', // not implemented yet
],
skip: ['reactTestRenderer', 'propsSpread', 'componentProp', 'componentsProp'],
}));

describe('keyboard navigation', () => {
Expand Down
30 changes: 20 additions & 10 deletions packages/mui-base/src/MenuUnstyled/MenuUnstyled.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import * as React from 'react';
import PropTypes from 'prop-types';
import clsx from 'clsx';
import { HTMLElementType, refType } from '@mui/utils';
import { HTMLElementType, refType, unstable_useForkRef as useForkRef } from '@mui/utils';
import appendOwnerState from '../utils/appendOwnerState';
import MenuUnstyledContext, { MenuUnstyledContextType } from './MenuUnstyledContext';
import {
Expand All @@ -15,6 +15,7 @@ import useMenu from './useMenu';
import composeClasses from '../composeClasses';
import PopperUnstyled from '../PopperUnstyled';
import { WithOptionalOwnerState } from '../utils';
import resolveComponentProps from '../utils/resolveComponentProps';

function getUtilityClasses(ownerState: MenuUnstyledOwnerState) {
const { open } = ownerState;
Expand Down Expand Up @@ -48,6 +49,7 @@ const MenuUnstyled = React.forwardRef(function MenuUnstyled(
components = {},
componentsProps = {},
keepMounted = false,
listboxId,
onClose,
open = false,
...other
Expand All @@ -64,8 +66,7 @@ const MenuUnstyled = React.forwardRef(function MenuUnstyled(
} = useMenu({
open,
onClose,
listboxRef: componentsProps.listbox?.ref,
listboxId: componentsProps.listbox?.id,
listboxId,
});

React.useImperativeHandle(
Expand All @@ -85,6 +86,7 @@ const MenuUnstyled = React.forwardRef(function MenuUnstyled(
const classes = getUtilityClasses(ownerState);

const Popper = component ?? components.Root ?? PopperUnstyled;
const popperComponentProps = resolveComponentProps(componentsProps.root, ownerState);
const popperProps: MenuUnstyledRootSlotProps = appendOwnerState(
Popper,
{
Expand All @@ -93,19 +95,23 @@ const MenuUnstyled = React.forwardRef(function MenuUnstyled(
open,
keepMounted,
role: undefined,
...componentsProps.root,
className: clsx(classes.root, className, componentsProps.root?.className),
...popperComponentProps,
className: clsx(classes.root, className, popperComponentProps?.className),
ref: useForkRef(popperComponentProps?.ref, forwardedRef),
},
ownerState,
) as MenuUnstyledRootSlotProps;

const Listbox = components.Listbox ?? 'ul';
const listboxComponentProps = resolveComponentProps(componentsProps.listbox, ownerState);
const propsFromHook = getListboxProps();
const listboxProps: WithOptionalOwnerState<MenuUnstyledListboxSlotProps> = appendOwnerState(
Listbox,
{
...componentsProps.listbox,
...getListboxProps(),
className: clsx(classes.listbox, componentsProps.listbox?.className),
...propsFromHook,
Copy link
Member

Choose a reason for hiding this comment

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

I like that this was resolved previously it helps with readability :)

...listboxComponentProps,
className: clsx(classes.listbox, listboxComponentProps?.className),
ref: useForkRef(propsFromHook.ref, listboxComponentProps?.ref),
},
ownerState,
);
Expand Down Expand Up @@ -170,8 +176,8 @@ MenuUnstyled.propTypes /* remove-proptypes */ = {
* @ignore
*/
componentsProps: PropTypes.shape({
listbox: PropTypes.object,
root: PropTypes.object,
listbox: PropTypes.oneOfType([PropTypes.func, PropTypes.object]),
root: PropTypes.oneOfType([PropTypes.func, PropTypes.object]),
}),
/**
* Always keep the menu in the DOM.
Expand All @@ -180,6 +186,10 @@ MenuUnstyled.propTypes /* remove-proptypes */ = {
* @default false
*/
keepMounted: PropTypes.bool,
/**
* @ignore
*/
listboxId: PropTypes.string,
/**
* Triggered when focus leaves the menu and the menu should close.
*/
Expand Down
15 changes: 12 additions & 3 deletions packages/mui-base/src/MenuUnstyled/MenuUnstyled.types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import React from 'react';
import PopperUnstyled, { PopperUnstyledProps } from '../PopperUnstyled';
import { SlotComponentProps } from '../utils';
import { UseMenuListboxSlotProps } from './useMenu.types';

export interface MenuUnstyledComponentsPropsOverrides {}
Expand Down Expand Up @@ -29,9 +30,16 @@ export interface MenuUnstyledProps {
Listbox?: React.ElementType;
};
componentsProps?: {
root?: Partial<React.ComponentPropsWithRef<typeof PopperUnstyled>> &
MenuUnstyledComponentsPropsOverrides;
listbox?: React.ComponentPropsWithRef<'ul'> & MenuUnstyledComponentsPropsOverrides;
root?: SlotComponentProps<
typeof PopperUnstyled,
MenuUnstyledComponentsPropsOverrides,
MenuUnstyledOwnerState
>;
listbox?: SlotComponentProps<
'ul',
MenuUnstyledComponentsPropsOverrides,
MenuUnstyledOwnerState
>;
};
/**
* Always keep the menu in the DOM.
Expand All @@ -40,6 +48,7 @@ export interface MenuUnstyledProps {
* @default false
*/
keepMounted?: boolean;
listboxId?: string;
/**
* Triggered when focus leaves the menu and the menu should close.
*/
Expand Down
34 changes: 22 additions & 12 deletions packages/mui-base/src/MenuUnstyled/useMenu.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,12 @@ import {
useListbox,
ActionTypes,
} from '../ListboxUnstyled';
import { MenuItemMetadata, MenuItemState, UseMenuParameters } from './useMenu.types';
import {
MenuItemMetadata,
MenuItemState,
UseMenuListboxSlotProps,
UseMenuParameters,
} from './useMenu.types';
import { EventHandlers } from '../utils';

function stateReducer(
Expand Down Expand Up @@ -38,7 +43,7 @@ function stateReducer(
return newState;
}

export default function useMenu(parameters: UseMenuParameters) {
export default function useMenu(parameters: UseMenuParameters = {}) {
const { listboxRef: listboxRefProp, open = false, onClose, listboxId } = parameters;

const [menuItems, setMenuItems] = React.useState<Record<string, MenuItemMetadata>>({});
Expand Down Expand Up @@ -97,8 +102,8 @@ export default function useMenu(parameters: UseMenuParameters) {
}
}, [open, highlightFirstItem]);

const createHandleKeyDown = (otherHandlers?: EventHandlers) => (e: React.KeyboardEvent) => {
otherHandlers?.onKeyDown?.(e);
const createHandleKeyDown = (otherHandlers: EventHandlers) => (e: React.KeyboardEvent) => {
otherHandlers.onKeyDown?.(e);
if (e.defaultPrevented) {
return;
}
Expand All @@ -108,8 +113,8 @@ export default function useMenu(parameters: UseMenuParameters) {
}
};

const createHandleBlur = (otherHandlers?: EventHandlers) => (e: React.FocusEvent) => {
otherHandlers?.onBlur(e);
const createHandleBlur = (otherHandlers: EventHandlers) => (e: React.FocusEvent) => {
otherHandlers.onBlur?.(e);

if (!listboxRef.current?.contains(e.relatedTarget)) {
onClose?.();
Expand All @@ -123,15 +128,20 @@ export default function useMenu(parameters: UseMenuParameters) {
}
}, [highlightedOption, menuItems]);

const getListboxProps = (otherHandlers?: EventHandlers) => ({
...otherHandlers,
...getRootProps({
const getListboxProps = <TOther extends EventHandlers>(
otherHandlers: TOther = {} as TOther,
): UseMenuListboxSlotProps => {
const rootProps = getRootProps({
...otherHandlers,
onBlur: createHandleBlur(otherHandlers),
onKeyDown: createHandleKeyDown(otherHandlers),
}),
role: 'menu',
});
});
return {
...otherHandlers,
...rootProps,
role: 'menu',
};
};

const getItemState = (id: string): MenuItemState => {
const { disabled, highlighted } = getOptionState(id);
Expand Down
9 changes: 6 additions & 3 deletions packages/mui-base/src/MenuUnstyled/useMenu.types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ import { UseListboxRootSlotProps } from '../ListboxUnstyled';
export interface MenuItemMetadata {
id: string;
disabled: boolean;
ref: React.RefObject<HTMLElement>;
label?: string;
ref: React.RefObject<HTMLElement>;
}

export interface MenuItemState {
Expand All @@ -17,7 +17,7 @@ export interface UseMenuParameters {
open?: boolean;
onClose?: () => void;
listboxId?: string;
listboxRef?: React.Ref<HTMLElement>;
listboxRef?: React.Ref<any>;
}

interface UseMenuListboxSlotEventHandlers {
Expand All @@ -27,4 +27,7 @@ interface UseMenuListboxSlotEventHandlers {

export type UseMenuListboxSlotProps<TOther = {}> = UseListboxRootSlotProps<
Omit<TOther, keyof UseMenuListboxSlotEventHandlers> & UseMenuListboxSlotEventHandlers
> & { role: React.AriaRole };
> & {
ref: React.Ref<any>;
role: React.AriaRole;
};
6 changes: 4 additions & 2 deletions packages/mui-base/src/utils/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,7 @@ export type WithOptionalOwnerState<T extends { ownerState: unknown }> = Omit<T,
Partial<Pick<T, 'ownerState'>>;

export type SlotComponentProps<TSlotComponent extends React.ElementType, TOverrides, TOwnerState> =
| (React.ComponentPropsWithRef<TSlotComponent> & TOverrides)
| ((ownerState: TOwnerState) => React.ComponentPropsWithRef<TSlotComponent> & TOverrides);
| (Partial<React.ComponentPropsWithRef<TSlotComponent>> & TOverrides)
| ((
ownerState: TOwnerState,
) => Partial<React.ComponentPropsWithRef<TSlotComponent>> & TOverrides);