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(OverflowMenu): call closeMenu method only when menu is open #5336

Conversation

emyarod
Copy link
Member

@emyarod emyarod commented Feb 13, 2020

Closes #5079

This PR adds an open state check to the handleClickOutside method so that the click listener correctly fires only when the menu is open.

Prior to this change, the click handler would fire with every click outside of the component regardless of the component state, meaning that for every overflow menu on a page, each click would trigger a handleClickOutside function call even if the menus were closed. As shown in the demo provided in the original ticket, this led to the action of opening a single overflow menu taking over 4 seconds on a page containing 500 menu instances.

image

With this check for menu state, the time taken for a single menu to open on the same demo is reduced to 19 milliseconds.

Screen Shot 2020-02-12 at 6 03 05 PM

Changelog

Changed

  • overflow menu handleClickOutside method

Testing / Reviewing

To test this locally, you can recreate the demo by adding this as a story in storybook and comparing it to the demo in the original ticket:

const Container = () => (
  <div>
    <OverflowMenu>
      <OverflowMenuItem itemText="item 1" />
      <OverflowMenuItem itemText="item 2" />
    </OverflowMenu>
  </div>
);

const containers = [];
for (let i = 0; i < 500; i++) {
  containers.push(<Container />);
}

return <div>{containers}</div>;

@emyarod emyarod requested a review from asudoh February 13, 2020 00:13
@emyarod emyarod requested a review from a team as a code owner February 13, 2020 00:13
@ghost ghost requested a review from aledavila February 13, 2020 00:13
@netlify
Copy link

netlify bot commented Feb 13, 2020

Deploy preview for carbon-elements ready!

Built with commit bb8930e

https://deploy-preview-5336--carbon-elements.netlify.com

@netlify
Copy link

netlify bot commented Feb 13, 2020

Deploy preview for carbon-components-react ready!

Built with commit bb8930e

https://deploy-preview-5336--carbon-components-react.netlify.com

Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

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

Simple fix that greatly improves the performance 🎉 - Thanks @emyarod!

@joshblack
Copy link
Contributor

cc @vpicone

Copy link
Contributor

@vpicone vpicone left a comment

Choose a reason for hiding this comment

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

Awesome work @emyarod thanks for tackling this. A big improvement!

@asudoh
Copy link
Contributor

asudoh commented Feb 15, 2020

Got a green light from @emyarod to make this "ready to merge" state - Thanks @emyarod!

@tw15egan
Copy link
Collaborator

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create over 300 Tooltip or OverflowMenu, page is not responsive
6 participants