-
Notifications
You must be signed in to change notification settings - Fork 56
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
Render activity list items fully first when they enter viewport #2417
Render activity list items fully first when they enter viewport #2417
Conversation
I should really clean this up a little before merge, the new component can be its own file. And also we maybe should discuss an alternative: Extending the actual list items (CallAssignmentListItem, etc) with logic to wait with loading their data but render their own 'pending' state rather than relying on stopping their render as I did here. My current version was just quicker than modifying them all. Tell me what you prefer, I see some pros and cons with both. |
This is an impressive solution! There have been plans to make some major changes to the pages where these lists live, so I don't think it is necessary to make this perfect. I'll review properly, but just wanted to stop in real quick and let you know that you should feel no pressure to clean this up! |
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 have reviewed the code and I've tried the build. I confirmed that not all data is loaded on first draw, but more data is loaded as the user scrolls.
I'm going to approve it as is because I'm eager to get this merged. But please note my question about garbage collection below and if you believe that something needs to be done let me know and we can create a separate issue for that. 😊
return; | ||
} | ||
|
||
const observer = new IntersectionObserver((entries) => { |
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 so glad you introduced this solution. I was not aware of IntersectionObserver
(like I was with ResizeObserver
and observers in general) so I had to look it up, but it's covered by all browsers that we care about so I think it's the ultimate solution for us.
}); | ||
}); | ||
|
||
observer.observe(boxRef.current); |
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.
Do we need to unobserve or destroy the observer when the component unmounts, or will that be handled automatically by the browser when React removes the element from the DOM?
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.
From what I can find in discussions the intent is for garbage collection to deal with it if observer is no longer observing any elements (using weak references):
w3c/csswg-drafts#6397
Description
Wraps the items of activity list in an IntersectionObserver component that waits for them to be inside the viewport before rendering them (which defers loading until then as well).
Displays a 'pending' list item to look a little neater on loading flash, and also take about roughly the same space as the actual items would.
Related issues
Resolves #2396