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

Add scrollElement #556

Merged
merged 7 commits into from Feb 6, 2022
Merged

Add scrollElement #556

merged 7 commits into from Feb 6, 2022

Conversation

cvanem
Copy link
Contributor

@cvanem cvanem commented Feb 3, 2022

PR for #320

Adds a scrollElement input prop which mimics the useWindowScroll feature with the provided element.

See examples for usage:
scroll-element-grid-scroller
scroll-element-list-scroller

@petyosi
Copy link
Owner

petyosi commented Feb 3, 2022

@cvanem this looks great - I will test it a bit more over the weekend but the implementation is correct overall.

@petyosi
Copy link
Owner

petyosi commented Feb 4, 2022

Hey @cvanem,

I tested the branch, and wanted to confirm something with you. The invisible items are rendered are due to the nested scrolling, right?

image

Also, you can run yarn lint --fix and commit the changes? thanks!

@cvanem
Copy link
Contributor Author

cvanem commented Feb 4, 2022

@petyosi

Yes, I believe that is correct, but it wouldn't hurt to have more eyes on it to make sure it is correct.

I originally tried implementing it with only the viewable boundary, but I found that the window contents would not refresh if another outer scroll div (not scrollElement) was adjusted to bring the list within the viewport.

I experimented a bit with adding an IntersectionObserver to trigger a refresh callback when this scenario occurred and did have some limited success but didn't really like the solution or use of thresholds and wasn't too confident with it.

I then implemented getScrollElementInfo, which seems to work so I went with that. It would be good to have a few more eyes on that function to check it for accuracy.

@maxhelsel
Copy link

@petyosi @cvanem Implementation looks good and is inline with what we all discussed in the original feature request thread!

A quick thought - I have not tested or played around with it, but when you state the following:

I found that the window contents would not refresh if another outer scroll div (not scrollElement) was adjusted to bring the list within the viewport

Sounds like you're describing the Virtuoso component failing to update when dimensions/components change within the scroll element container? For example, you have a header or something that might render in and/or change size and skews the dimensions of the Virtuoso component relative to the viewport - therefore needing an update to recalculate dimensions.

The addition of getScrollElementInfo looks to be a fix, but I bring this up because I have been using React-Virtualized WindowScroller (RVWS) to allow for this entire ScrollElement use case in my virtualized lists, and that package has that same issue.

RVWS uses an outside method to address this very issue of updating dimensions, from the documentation:

updatePosition
Recalculates scroll position from the top of page.
This method is automatically triggered when the component mounts as well as when the browser resizes. It should be manually called if the page header (eg any items in the DOM "above" the WindowScroller) resizes or changes.

When using RVWS I found myself having to call that method whenever additional components rendered into the scroll element container and pushed the List down.

Again, your getScrollElementInfo solution seems to address this on-the-fly instead having to manually call it. However, it might be worth a quick look to see what's going on in the RVWS component code.

Otherwise, looks good to me. Hopefully I'll be able to drop React-Virtualized altogether soon!

src/components.tsx Outdated Show resolved Hide resolved
Co-authored-by: Petyo Ivanov <underlog@gmail.com>
@petyosi
Copy link
Owner

petyosi commented Feb 5, 2022

@cvanem what do you think about renaming the prop to externalScrollableParentElement? I know it's longer, but we have code autocomplete these days.

@cvanem
Copy link
Contributor Author

cvanem commented Feb 5, 2022

@cvanem what do you think about renaming the prop to externalScrollableParentElement? I know it's longer, but we have code autocomplete these days.

@petyosi I prefer short simple names but we can name it whatever you want. The documentation can describe the usage in further detail if needed.

@petyosi petyosi merged commit f37d05a into petyosi:master Feb 6, 2022
@github-actions
Copy link

github-actions bot commented Feb 6, 2022

🎉 This PR is included in version 2.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

3 participants