This repository has been archived by the owner on Mar 13, 2024. It is now read-only.
MM-41319 Reduced number of listeners registered by MenuWrapper #9707
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
larkox
approved these changes
Jan 27, 2022
| } | ||
|
|
||
| private addEventListeners() { | ||
| document.addEventListener('click', this.closeOnBlur, true); |
There was a problem hiding this comment.
nit: out of the scope of the pr, but what about moving the click and keyup events names to a constant? (May be in this file)
There was a problem hiding this comment.
Yeah, we could consider that. I don't personally think it's that helpful though since they're standard values that come from the browser. I'm not opposed to doing that, but I'm too lazy to go back and do it here 😛
M-ZubairAhmed
approved these changes
Jan 27, 2022
|
/update-branch |
|
/e2e-test |
|
Triggering e2e testing with options: |
|
Successfully triggered e2e testing! |
|
Test server destroyed |
|
/cherry-pick release-6.3 |
|
Cherry pick is scheduled. |
mattermost-build
pushed a commit
to mattermost-build/mattermost-webapp
that referenced
this pull request
Feb 11, 2022
…rmost#9707) * MM-41319 Reduced number of listeners registered by MenuWrapper * Fix linting Co-authored-by: Mattermod <[email protected]> (cherry picked from commit d500ab7)
hmhealey
added a commit
that referenced
this pull request
Feb 11, 2022
#9801) * MM-41319 Reduced number of listeners registered by MenuWrapper * Fix linting Co-authored-by: Mattermod <[email protected]> (cherry picked from commit d500ab7) Co-authored-by: Harrison Healey <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, every instance of MenuWrapper registers two listeners on the document that respond to every keyup and click event made in the app. With only a couple channels, this is already 12 listeners running on each key press or click, but when you have more MenuWrappers mounted (notably each sidebar channel and category uses one even if they're not visible), that starts to add up the amount of time taken to process those events.
Since both of those listeners are used to close the menu when it's open, I've changed those to only be registered once the menu gets opened and then removed when it's closed.
Ticket Link
https://mattermost.atlassian.net/browse/MM-41319
Related Pull Requests
TBD
Screenshots
I added log statements to the listeners to show when they trigger. Here's a before and after with a brand new account
Before:
before.mov
After:
after.mov
Release Note