fix: MenuList props should win over context props (#26252)

* fix: MenuList props should win over context props Fixes the regression introduced in #25672 so that prop values win over context props with the same name. Fixes # * changefile
microsoft · Jan 9, 2023 · 307d798 · 307d798
1 parent ab386c3
commit 307d798
Show file tree

Hide file tree

Showing 12 changed files with 342 additions and 26 deletions.
diff --git a/change/@fluentui-react-menu-7ad515c7-ae51-477d-9289-c01f7a691b06.json b/change/@fluentui-react-menu-7ad515c7-ae51-477d-9289-c01f7a691b06.json
@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "fix: MenuList props should win over context props",
+  "packageName": "@fluentui/react-menu",
+  "email": "lingfangao@hotmail.com",
+  "dependentChangeType": "patch"
+}
diff --git a/packages/react-components/react-menu/etc/react-menu.api.md b/packages/react-components/react-menu/etc/react-menu.api.md
@@ -211,12 +211,10 @@ export type MenuListSlots = {
 };
 
 // @public (undocumented)
-export type MenuListState = ComponentState<MenuListSlots> & Required<Pick<MenuListProps, 'checkedValues' | 'hasCheckmarks' | 'hasIcons'>> & {
+export type MenuListState = ComponentState<MenuListSlots> & Required<Pick<MenuListProps, 'checkedValues' | 'hasCheckmarks' | 'hasIcons'>> & Pick<MenuListProps, 'defaultCheckedValues' | 'onCheckedValueChange'> & {
     selectRadio: SelectableHandler;
     setFocusByFirstCharacter: NonNullable<MenuListContextValue['setFocusByFirstCharacter']>;
     toggleCheckbox: SelectableHandler;
-    defaultCheckedValues?: Record<string, string[]>;
-    onCheckedValueChange?: (e: MenuCheckedValueChangeEvent, data: MenuCheckedValueChangeData) => void;
 };
 
 // @public

diff --git a/packages/react-components/react-menu/src/components/Menu/Menu.test.tsx b/packages/react-components/react-menu/src/components/Menu/Menu.test.tsx
@@ -284,4 +284,118 @@ describe('Menu', () => {
     // Assert
     expect(container.querySelector('[role="menu"]')).not.toBeNull();
   });
+
+  it('should call onCheckedValueChange when item is selected', () => {
+    const onCheckedValueChange = jest.fn();
+    const { getAllByRole } = render(
+      <Menu open inline onCheckedValueChange={onCheckedValueChange}>
+        <MenuTrigger disableButtonEnhancement>
+          <button>MenuTrigger</button>
+        </MenuTrigger>
+
+        <MenuPopover>
+          <MenuList>
+            <MenuItemCheckbox name="test" value="first">
+              First
+            </MenuItemCheckbox>
+            <MenuItemCheckbox name="test" value="second">
+              Second
+            </MenuItemCheckbox>
+            <MenuItemCheckbox name="test" value="third">
+              Third
+            </MenuItemCheckbox>
+          </MenuList>
+        </MenuPopover>
+      </Menu>,
+    );
+
+    const checkboxes = getAllByRole('menuitemcheckbox');
+    fireEvent.click(checkboxes[0]);
+    expect(onCheckedValueChange).toHaveBeenCalledTimes(1);
+  });
+
+  it('should control checked items with checkedValues prop', () => {
+    const { container } = render(
+      <Menu open inline checkedValues={{ test: ['second'] }}>
+        <MenuTrigger disableButtonEnhancement>
+          <button>MenuTrigger</button>
+        </MenuTrigger>
+
+        <MenuPopover>
+          <MenuList>
+            <MenuItemCheckbox id="first" name="test" value="first">
+              First
+            </MenuItemCheckbox>
+            <MenuItemCheckbox id="second" name="test" value="second">
+              Second
+            </MenuItemCheckbox>
+            <MenuItemCheckbox id="third" name="test" value="third">
+              Third
+            </MenuItemCheckbox>
+          </MenuList>
+        </MenuPopover>
+      </Menu>,
+    );
+
+    expect(container.querySelector('#first')?.getAttribute('aria-checked')).toBe('false');
+    expect(container.querySelector('#second')?.getAttribute('aria-checked')).toBe('true');
+    expect(container.querySelector('#third')?.getAttribute('aria-checked')).toBe('false');
+  });
+
+  it('should call onCheckedValueChange (applied to MenuList) when item is selected', () => {
+    const onCheckedValueChange = jest.fn();
+    const { getAllByRole } = render(
+      <Menu open inline>
+        <MenuTrigger disableButtonEnhancement>
+          <button>MenuTrigger</button>
+        </MenuTrigger>
+
+        <MenuPopover>
+          <MenuList onCheckedValueChange={onCheckedValueChange}>
+            <MenuItemCheckbox name="test" value="first">
+              First
+            </MenuItemCheckbox>
+            <MenuItemCheckbox name="test" value="second">
+              Second
+            </MenuItemCheckbox>
+            <MenuItemCheckbox name="test" value="third">
+              Third
+            </MenuItemCheckbox>
+          </MenuList>
+        </MenuPopover>
+      </Menu>,
+    );
+
+    const checkboxes = getAllByRole('menuitemcheckbox');
+    fireEvent.click(checkboxes[0]);
+    expect(onCheckedValueChange).toHaveBeenCalledTimes(1);
+  });
+
+  it('should control checked items with checkedValues prop (applied to MenuList)', () => {
+    const { container } = render(
+      <Menu open inline>
+        <MenuTrigger disableButtonEnhancement>
+          <button>MenuTrigger</button>
+        </MenuTrigger>
+
+        <MenuPopover>
+          <MenuList checkedValues={{ test: ['second'] }}>
+            <MenuItemCheckbox id="first" name="test" value="first">
+              First
+            </MenuItemCheckbox>
+            <MenuItemCheckbox id="second" name="test" value="second">
+              Second
+            </MenuItemCheckbox>
+            <MenuItemCheckbox id="third" name="test" value="third">
+              Third
+            </MenuItemCheckbox>
+          </MenuList>
+        </MenuPopover>
+      </Menu>,
+    );
+
+    expect(container.querySelector('#first')?.getAttribute('aria-checked')).toBe('false');
+    expect(container.querySelector('#second')?.getAttribute('aria-checked')).toBe('true');
+    expect(container.querySelector('#third')?.getAttribute('aria-checked')).toBe('false');
+  });
 });
diff --git a/packages/react-components/react-menu/src/components/MenuList/MenuList.types.ts b/packages/react-components/react-menu/src/components/MenuList/MenuList.types.ts
@@ -47,7 +47,8 @@ export type MenuListProps = ComponentProps<MenuListSlots> & {
 };
 
 export type MenuListState = ComponentState<MenuListSlots> &
-  Required<Pick<MenuListProps, 'checkedValues' | 'hasCheckmarks' | 'hasIcons'>> & {
+  Required<Pick<MenuListProps, 'checkedValues' | 'hasCheckmarks' | 'hasIcons'>> &
+  Pick<MenuListProps, 'defaultCheckedValues' | 'onCheckedValueChange'> & {
     /**
      * Selects a radio item, will de-select the currently selected ratio item
      */
@@ -62,22 +63,6 @@ export type MenuListState = ComponentState<MenuListSlots> &
      * Toggles the state of a checkbox item
      */
     toggleCheckbox: SelectableHandler;
-
-    /**
-     * Default values to be checked on mount
-     * @deprecated this property is not used internally anymore,
-     * the signature remains just to avoid breaking changes
-     */
-    defaultCheckedValues?: Record<string, string[]>;
-    /**
-     * Callback when checked items change for value with a name
-     *
-     * @param event - React's original SyntheticEvent
-     * @param data - A data object with relevant information
-     * @deprecated this property is not used internally anymore,
-     * the signature remains just to avoid breaking changes
-     */
-    onCheckedValueChange?: (e: MenuCheckedValueChangeEvent, data: MenuCheckedValueChangeData) => void;
   };
 
 export type MenuListContextValues = {

diff --git a/packages/react-components/react-menu/src/components/MenuList/useMenuList.ts b/packages/react-components/react-menu/src/components/MenuList/useMenuList.ts
@@ -75,12 +75,13 @@ export const useMenuList_unstable = (props: MenuListProps, ref: React.Ref<HTMLEl
   );
 
   const [checkedValues, setCheckedValues] = useControllableState({
-    state: hasMenuContext ? menuContext.checkedValues : props.checkedValues,
+    state: props.checkedValues ?? (hasMenuContext ? menuContext.checkedValues : undefined),
     defaultState: props.defaultCheckedValues,
     initialState: {},
   });
 
-  const handleCheckedValueChange = hasMenuContext ? menuContext.onCheckedValueChange : props.onCheckedValueChange;
+  const handleCheckedValueChange =
+    props.onCheckedValueChange ?? (hasMenuContext ? menuContext.onCheckedValueChange : undefined);
 
   const toggleCheckbox = useEventCallback(
     (e: React.MouseEvent | React.KeyboardEvent, name: string, value: string, checked: boolean) => {

diff --git a/packages/react-components/react-menu/stories/Menu/MenuControlledCheckboxItems.stories.tsx b/packages/react-components/react-menu/stories/Menu/MenuControlledCheckboxItems.stories.tsx
@@ -25,12 +25,12 @@ export const ControlledCheckboxItems = () => {
   };
 
   return (
-    <Menu>
+    <Menu checkedValues={checkedValues} onCheckedValueChange={onChange}>
       <MenuTrigger disableButtonEnhancement>
         <Button>Toggle menu</Button>
       </MenuTrigger>
       <MenuPopover>
-        <MenuList checkedValues={checkedValues} onCheckedValueChange={onChange}>
+        <MenuList>
           <MenuItemCheckbox icon={<CutIcon />} name="edit" value="cut">
             Cut
           </MenuItemCheckbox>

diff --git a/packages/react-components/react-menu/stories/Menu/MenuControlledRadioItems.stories.tsx b/packages/react-components/react-menu/stories/Menu/MenuControlledRadioItems.stories.tsx
@@ -23,12 +23,12 @@ export const ControlledRadioItems = () => {
   };
 
   return (
-    <Menu>
+    <Menu checkedValues={checkedValues} onCheckedValueChange={onChange}>
       <MenuTrigger disableButtonEnhancement>
         <Button>Toggle menu</Button>
       </MenuTrigger>
       <MenuPopover>
-        <MenuList checkedValues={checkedValues} onCheckedValueChange={onChange}>
+        <MenuList>
           <MenuItemRadio icon={<CutIcon />} name="font" value="segoe">
             Segoe
           </MenuItemRadio>

diff --git a/packages/react-components/react-menu/stories/MenuList/MenuListCheckboxItems.stories.tsx b/packages/react-components/react-menu/stories/MenuList/MenuListCheckboxItems.stories.tsx
@@ -0,0 +1,49 @@
+import * as React from 'react';
+
+import { MenuList, MenuItemCheckbox, makeStyles, tokens } from '@fluentui/react-components';
+import {
+  bundleIcon,
+  CutRegular,
+  CutFilled,
+  ClipboardPasteRegular,
+  ClipboardPasteFilled,
+  EditRegular,
+  EditFilled,
+} from '@fluentui/react-icons';
+
+const CutIcon = bundleIcon(CutFilled, CutRegular);
+const PasteIcon = bundleIcon(ClipboardPasteFilled, ClipboardPasteRegular);
+const EditIcon = bundleIcon(EditFilled, EditRegular);
+
+export const useMenuListContainerStyles = makeStyles({
+  container: {
+    backgroundColor: tokens.colorNeutralBackground1,
+    minWidth: '128px',
+    minHeight: '48px',
+    maxWidth: '300px',
+    width: 'max-content',
+    boxShadow: `${tokens.shadow16}`,
+    paddingTop: '4px',
+    paddingBottom: '4px',
+  },
+});
+
+export const CheckboxItems = () => {
+  const styles = useMenuListContainerStyles();
+
+  return (
+    <div className={styles.container}>
+      <MenuList>
+        <MenuItemCheckbox icon={<CutIcon />} name="edit" value="cut">
+          Cut
+        </MenuItemCheckbox>
+        <MenuItemCheckbox icon={<PasteIcon />} name="edit" value="paste">
+          Paste
+        </MenuItemCheckbox>
+        <MenuItemCheckbox icon={<EditIcon />} name="edit" value="edit">
+          Edit
+        </MenuItemCheckbox>
+      </MenuList>
+    </div>
+  );
+};
diff --git a/.../react-components/react-menu/stories/MenuList/MenuListControlledCheckboxItems.stories.tsx b/.../react-components/react-menu/stories/MenuList/MenuListControlledCheckboxItems.stories.tsx
@@ -0,0 +1,56 @@
+import * as React from 'react';
+
+import { MenuList, MenuItemCheckbox, makeStyles, tokens } from '@fluentui/react-components';
+import {
+  bundleIcon,
+  CutRegular,
+  CutFilled,
+  ClipboardPasteRegular,
+  ClipboardPasteFilled,
+  EditRegular,
+  EditFilled,
+} from '@fluentui/react-icons';
+import type { MenuProps } from '@fluentui/react-components';
+
+const CutIcon = bundleIcon(CutFilled, CutRegular);
+const PasteIcon = bundleIcon(ClipboardPasteFilled, ClipboardPasteRegular);
+const EditIcon = bundleIcon(EditFilled, EditRegular);
+
+export const useMenuListContainerStyles = makeStyles({
+  container: {
+    backgroundColor: tokens.colorNeutralBackground1,
+    minWidth: '128px',
+    minHeight: '48px',
+    maxWidth: '300px',
+    width: 'max-content',
+    boxShadow: `${tokens.shadow16}`,
+    paddingTop: '4px',
+    paddingBottom: '4px',
+  },
+});
+
+export const ControlledCheckboxItems = () => {
+  const styles = useMenuListContainerStyles();
+  const [checkedValues, setCheckedValues] = React.useState<Record<string, string[]>>({ edit: ['cut', 'paste'] });
+  const onChange: MenuProps['onCheckedValueChange'] = (e, { name, checkedItems }) => {
+    setCheckedValues(s => {
+      return s ? { ...s, [name]: checkedItems } : { [name]: checkedItems };
+    });
+  };
+
+  return (
+    <div className={styles.container}>
+      <MenuList checkedValues={checkedValues} onCheckedValueChange={onChange}>
+        <MenuItemCheckbox icon={<CutIcon />} name="edit" value="cut">
+          Cut
+        </MenuItemCheckbox>
+        <MenuItemCheckbox icon={<PasteIcon />} name="edit" value="paste">
+          Paste
+        </MenuItemCheckbox>
+        <MenuItemCheckbox icon={<EditIcon />} name="edit" value="edit">
+          Edit
+        </MenuItemCheckbox>
+      </MenuList>
+    </div>
+  );
+};
diff --git a/...ges/react-components/react-menu/stories/MenuList/MenuListControlledRadioItems.stories.tsx b/...ges/react-components/react-menu/stories/MenuList/MenuListControlledRadioItems.stories.tsx
@@ -0,0 +1,54 @@
+import * as React from 'react';
+
+import { MenuList, MenuItemRadio, makeStyles, tokens } from '@fluentui/react-components';
+import {
+  bundleIcon,
+  CutRegular,
+  CutFilled,
+  ClipboardPasteRegular,
+  ClipboardPasteFilled,
+  EditRegular,
+  EditFilled,
+} from '@fluentui/react-icons';
+import type { MenuProps } from '@fluentui/react-components';
+
+const CutIcon = bundleIcon(CutFilled, CutRegular);
+const PasteIcon = bundleIcon(ClipboardPasteFilled, ClipboardPasteRegular);
+const EditIcon = bundleIcon(EditFilled, EditRegular);
+
+export const useMenuListContainerStyles = makeStyles({
+  container: {
+    backgroundColor: tokens.colorNeutralBackground1,
+    minWidth: '128px',
+    minHeight: '48px',
+    maxWidth: '300px',
+    width: 'max-content',
+    boxShadow: `${tokens.shadow16}`,
+    paddingTop: '4px',
+    paddingBottom: '4px',
+  },
+});
+
+export const ControlledRadioItems = () => {
+  const styles = useMenuListContainerStyles();
+  const [checkedValues, setCheckedValues] = React.useState<Record<string, string[]>>({ font: ['calibri'] });
+  const onChange: MenuProps['onCheckedValueChange'] = (e, { name, checkedItems }) => {
+    setCheckedValues(s => ({ ...s, [name]: checkedItems }));
+  };
+
+  return (
+    <div className={styles.container}>
+      <MenuList checkedValues={checkedValues} onCheckedValueChange={onChange}>
+        <MenuItemRadio icon={<CutIcon />} name="font" value="segoe">
+          Segoe
+        </MenuItemRadio>
+        <MenuItemRadio icon={<PasteIcon />} name="font" value="calibri">
+          Calibri
+        </MenuItemRadio>
+        <MenuItemRadio icon={<EditIcon />} name="font" value="arial">
+          Arial
+        </MenuItemRadio>
+      </MenuList>
+    </div>
+  );
+};