fix: Whatever selectable, should close it (#401)

* fix: Whatever selectable, should close it

* test: More test case

Author: zombieJ

src/Menu.tsx

@@ -331,41 +331,40 @@ const Menu: React.FC<MenuProps> = props => {
 
   // >>>>> Trigger select
   const triggerSelection = (info: MenuInfo) => {
-    if (!selectable) {
-      return;
-    }
-
-    // Insert or Remove
-    const { key: targetKey } = info;
-    const exist = mergedSelectKeys.includes(targetKey);
-    let newSelectKeys: string[];
-
-    if (multiple) {
-      if (exist) {
-        newSelectKeys = mergedSelectKeys.filter(key => key !== targetKey);
+    if (selectable) {
+      // Insert or Remove
+      const { key: targetKey } = info;
+      const exist = mergedSelectKeys.includes(targetKey);
+      let newSelectKeys: string[];
+
+      if (multiple) {
+        if (exist) {
+          newSelectKeys = mergedSelectKeys.filter(key => key !== targetKey);
+        } else {
+          newSelectKeys = [...mergedSelectKeys, targetKey];
+        }
       } else {
-        newSelectKeys = [...mergedSelectKeys, targetKey];
+        newSelectKeys = [targetKey];
       }
-    } else {
-      newSelectKeys = [targetKey];
-    }
 
-    setMergedSelectKeys(newSelectKeys);
+      setMergedSelectKeys(newSelectKeys);
 
-    // Trigger event
-    const selectInfo: SelectInfo = {
-      ...info,
-      selectedKeys: newSelectKeys,
-    };
+      // Trigger event
+      const selectInfo: SelectInfo = {
+        ...info,
+        selectedKeys: newSelectKeys,
+      };
 
-    if (exist) {
-      onDeselect?.(selectInfo);
-    } else {
-      onSelect?.(selectInfo);
+      if (exist) {
+        onDeselect?.(selectInfo);
+      } else {
+        onSelect?.(selectInfo);
+      }
     }
 
+    // Whatever selectable, always close it
     if (!multiple && mergedOpenKeys.length) {
-      onOpenChange?.(EMPTY_LIST);
+      triggerOpenKeys(EMPTY_LIST);
     }
   };
 

tests/Menu.spec.js

@@ -540,31 +540,43 @@ describe('Menu', () => {
     errorSpy.mockRestore();
   });
 
-  it('Click should close Menu', async () => {
-    jest.useFakeTimers();
+  describe('Click should close Menu', () => {
+    function test(name, props) {
+      it(name, async () => {
+        jest.useFakeTimers();
+
+        const onOpenChange = jest.fn();
+
+        const wrapper = mount(
+          <Menu
+            openKeys={['bamboo']}
+            mode="vertical"
+            onOpenChange={onOpenChange}
+            {...props}
+          >
+            <SubMenu key="bamboo" title="Bamboo">
+              <MenuItem key="light">Light</MenuItem>
+            </SubMenu>
+          </Menu>,
+        );
 
-    const onOpenChange = jest.fn();
+        // Open menu
+        await act(async () => {
+          jest.runAllTimers();
+          wrapper.update();
+        });
 
-    const wrapper = mount(
-      <Menu openKeys={['bamboo']} mode="vertical" onOpenChange={onOpenChange}>
-        <SubMenu key="bamboo" title="Bamboo">
-          <MenuItem key="light">Light</MenuItem>
-        </SubMenu>
-      </Menu>,
-    );
+        wrapper.update();
 
-    // Open menu
-    await act(async () => {
-      jest.runAllTimers();
-      wrapper.update();
-    });
-
-    wrapper.update();
+        wrapper.find('.rc-menu-item').last().simulate('click');
+        expect(onOpenChange).toHaveBeenCalledWith([]);
 
-    wrapper.find('.rc-menu-item').last().simulate('click');
-    expect(onOpenChange).toHaveBeenCalledWith([]);
+        jest.useRealTimers();
+      });
+    }
 
-    jest.useRealTimers();
+    test('basic');
+    test('not selectable', { selectable: false });
   });
 });
 /* eslint-enable */