-
-
Notifications
You must be signed in to change notification settings - Fork 225
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
perf: custom DOM recycle #606
Conversation
a637f31
to
56e3a7d
Compare
Codecov Report
@@ Coverage Diff @@
## master #606 +/- ##
==========================================
- Coverage 12.12% 11.43% -0.69%
==========================================
Files 134 134
Lines 3523 3788 +265
Branches 523 564 +41
==========================================
+ Hits 427 433 +6
- Misses 3073 3332 +259
Partials 23 23
Continue to review full report at Codecov.
|
56e3a7d
to
e32e96a
Compare
e32e96a
to
68a15e4
Compare
@MrTimscampi @camc314 @ThibaultNocchi All the issues that the original implementation had are now fixed and I believe this is already in an state where it can 100% replace our current virtual scroller. Some obscure bugs I couldn't reproduce through this week might exist though, however, they're probably easily fixable until we release, the advantage we have of being an alpha client :). |
Sonarcloud is reporting a bug because that property is still not standardized and only Chromium-based browsers use it. However, it shouldn't cause any issues in Firefox, as it's only mission is to change how painting process is laid out, visually there are no differences. Check this for more info: #274 (comment) |
targetCards: { | ||
type: Number, | ||
required: false, | ||
default: 150 |
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 tested the following values: 80, 150, 200. 80 gives the best performance while having enough buffer space for light scrolling, 150 seems the sweetest spot for me and 200 gives near native scrolling appearance but, ofc, it's the most intensive of all the others.
Test your values and report back
instances.forEach((instance) => { | ||
instance.$destroy(); | ||
}); |
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.
Important to check as well
15fbc17
to
038fe2a
Compare
038fe2a
to
caeb120
Compare
@@ -0,0 +1,18 @@ | |||
<template> |
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.
What's the point in this file?
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.
Although in single-file templates mutating the props of the children components work and it's reactive, in this DOM recyling approach we're working with real instances. Thus, we can't mutate props programatically, we can only mutate data (console errors appears and nothing happens).
This component serves as a wrapper of the card one (which takes item as a prop), but it has no props and takes all BaseItemDto using it's data. That way, we keep backwards compatibility for places where we don't need virtual loaders, like item pages, home page, etc...
caeb120
to
e95ebda
Compare
e95ebda
to
560c5cc
Compare
SonarCloud Quality Gate failed. 1 Bug No Coverage information |
Issues go stale after 60 days of inactivity. Mark the issue as fresh by adding a comment or commit. Stale issues close after an additional 14 days of inactivity. If this issue is safe to close now please do so. If you have any questions you can reach us on Matrix or Social Media. |
Custom DOM recycle as discussed in #274
Features
items
array mutation and commits the changes only to the visible items that are changed. Unchanged items remains altered, while previous implementation required the same row to be changed. Applying changes to specific items are untested though, as there was no way to test that at our current stage but now that we have search finished I need to make sure this 100% worksDisadvantages
The first row is the one used for taking all the measurements of size. We have some views that mix cards with different shapes and they're not going to work well
Issues
This is already good enough to be reviewed, as I tested this a lot for weeks. However, there are still issues that does make this not a 100% perfect replacement. I already know the cause of most of them and they're easy to solve, but I want to have some feedback from the community about the whole current implementation before working on this, so, in case nobody likes it, fixing stuff doesn't end up as a completely wasted effort
After resizing the window while the scroll is not at the beginning of the page, some side effects might take place. Not always noticeable but it happens in pages with lots of items.The process to create additional cards when the amount oftargetCards
is less than the amount of cards that should be visible doesn't take place after the component has been mounted, only before. To reproduce:SettargetCards
to 100. Zoom out your browser to 25%. Go to a library and see how additional cards are created to cover the empty space at the bottom that the zooms leaves.Reload the page at 100% zoom and, after it has been mounted, zoom out at 25%: additional cards should be created but they aren'tLight scrolls (two key presses to the arrows in the keyboard for instance) that change direction many times in a row might lead for swaps that happens when scrolling slower to have visual artifacts.When scrolling up, first row might not keep the correct order.All fixed now
Please, test as much as you can stuff like the last one: items out of order, items swapping order in the row while scrolling, etc...
To discuss
While developing this, I had the
targetCards
prop set to 100. It was already much better than the current solution, but you could say cards "loading" if scrolling way too fast. Later days of testing I set it to 200 items (which I think it's still good enough for the majority of devices) to allow faster scrolling without noticing that "loading" and it's near native scrolling. What are you opinions on this value?Comparison
All the screrenshots were taken using the following approach:
Before
With this implementation (
targetCards
set to 200)With this implementation (
targetCards
set to 100)Notice also how not only scripting time is reduced, but also memory heap
Further improvements to performance
Performance might be even better if we do the following (not for this PR though):
mouseover
events that are fired if the cursor is in the middle. Should be improved in Vuetify 3 anyway