-
Notifications
You must be signed in to change notification settings - Fork 912
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
Debounce the layout function in the list component #1928
Debounce the layout function in the list component #1928
Conversation
when removing an item
Performance gainsThe following measurements were taken for the test (test:debug, http://localhost:9876/debug.html?grep=mwc-list%3A%20mwc-list%20performance%20issue) before and after applying the change. |
Caveat: The debounced updates are no longer part of the lists'updateComplete hook. Can try to add it to the _getUpdateComplete somehow if needs be. Another solution can be to allow the user to add a flag asyncRemoval or syncRemoval, and commence the layout accordingly. Let me know if and how you'd like to proceed with this one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please update CHANGELOG.md
super thanks for the PR! |
This seems like a simple enough implementation so let's go with this for now. Part of our code submission process includes importing this code into google's monorepo, and google-wide tests should help us run into migration issues other users may also have; this is where we can determine whether we need a flag. Though hooking into getUpdateComplete might be a requirement |
@e111077 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
final stretch! Great work so far!
Protect the itemsReadyResolver
fd72948
to
5d1284e
Compare
536cc9a
to
8d3b2e3
Compare
Resolved all mentioned issues 😀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also debugging a failure in a YouTube test that uses select. A little afraid it may be caused by the debounce vs a buffer, but it's still too early to tell
It is a breaking change, as it changes the interface (from sync to async). So it might be this test assumes the remove event should have sync effect. |
Haven't forgotten about this; still trying to untangle their esoteric code |
when removing an item.
Closes #1927
See my remarks blow regarding the outcome of this performance improvement, and proposed changes to this PR.