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

Fix the issue where the Menu will open when clicking anywhere on the screen other than the target #4114

Merged
merged 1 commit into from
Apr 25, 2023

Conversation

xshuxin
Copy link
Contributor

@xshuxin xshuxin commented Apr 21, 2023

When clicking anywhere on the screen that is not a target, the Menu also performs opening and closing.
https://codesandbox.io/s/nifty-curran-fy1nlu?file=/src/App.tsx

Based on my humble opinion, it may be caused by two reasons:

image

  • The dropdown is only hidden and has not been completely uninstalled, resulting in its lifecycle methods and other methods still existing.

  • clickOutside is not controlled based on the opened state, so both the open and closed states will trigger onClose.

In version 6.0.8, in order for Menu to be properly controlled and simultaneously call onClose and onOpen, I made clickOutside call the toggleDropdown method of Menu, resulting in a situation where clicking on an external button can also control the switch state.

Here are the fixes I made:
clickOutside only executes the onClose method when opened: true.

useClickOutside(() => opened && closeOnClickOutside && popover.onClose(), clickOutsideEvents, [
    targetNode,
    dropdownNode,
  ]) 

At the same time, I also modified the test file for closeOnClickOutside in Popover:

  it('correctly handles closeOnClickOutside={false}', async () => {
   const spy = jest.fn();
   render(<TestContainer opened closeOnClickOutside={false} onClose={spy} />);
   await userEvent.click(document.body);
   expect(spy).not.toHaveBeenCalled();
 });

 it('correctly handles closeOnClickOutside={true}', async () => {
   const spy = jest.fn();
   render(<TestContainer opened closeOnClickOutside onClose={spy} />);
   await userEvent.click(document.body);
   expect(spy).toHaveBeenCalled();
 });

Finally, I have added detailed testing files for Menu in both controlled and uncontrolled states. Feel free to rework or adjust them~

@rtivital rtivital merged commit 6f72238 into mantinedev:master Apr 25, 2023
1 check passed
@rtivital
Copy link
Member

Thanks!

@rtivital
Copy link
Member

@xshuxin This PR broke Popover component click outside logic, see #4146

@xshuxin xshuxin deleted the feat/menu-click-outside branch April 27, 2023 05:44
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.

None yet

2 participants