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

Further align web List with FlatList, add contain mode to web list implementation #3867

Merged
merged 27 commits into from
May 6, 2024

Conversation

haileyok
Copy link
Contributor

@haileyok haileyok commented May 5, 2024

Stacked on #3866

Why

Without whitespace

Right now, our web list implementation only fully supports the the assumption that the list will only be used in a way that monitoring window scroll events will suffice. This means that, for example in our chat implementation, we cannot have a List that works inside of its own scrollable container within our window. This breaks usages that we could currently achieve with FlatList.

Of course, we could just use FlatList directly in these cases. This still is not a great solution though, because we lose the improvements that were made in the web list implementation like synchronous layout measurements and scroll events.

I am also adding the following FlatList props and functions to support our chat implementation:

  • scrollToEnd() - This is a method that is available on the native FlatList but is missing from our implementation.
  • Add layoutMeasurement and contentSize to the onScroll event.

How

Container Mode

If we set the list to use container "mode" (using the containWeb prop), we add a overflow-y prop to the list's styles. The end result is that instead of our window becoming scrollable that the <List> itself becomes scrollable.

Screenshot 2024-05-05 at 12 49 04 AM

For the most part, the current implementation already supports everything when switched to this mode. However, there are a few additions we ned to make when it comes to handling scroll callbacks. Because we are now scrolling a specific element rather than the window, we need to attach the scroll listener to the div, which we already have a ref for called nativeRef.

Elsewhere, we have to fork logic depending on if we are in container mode or not. For example, the contentSize object in the onScroll callback is achieved this way in container mode:

          contentSize: {
            width: element?.scrollWidth,
            height: element?.scrollHeight,
          },

But when in "regular mode", we have to get this from the document:

          contentSize: {
            width: document.documentElement.scrollWidth,
            height: document.documentElement.scrollHeight,
          },

scrollToEnd()

This is pretty basic. We just scroll to an offset of whatever the scroll height is. Again, depending on if we are using contain or "full" mode, we either use element?.scrollHeight or document.documentElement.scrollHeight.

Test Plan

I have added a contained list to the storybook. To access it, scroll to the bottom of the storybook page. Observe the following in the console:

  • onStartReached should fire immediately, since we start at the top of the list (this is normal FlatList behavior)
  • onEndReached should fire within 2 pages - based on the height of the contained list - of the end.
  • onScroll should print throughout the scrolling you perform

Additionally, you can test using this PR: #3866

Behavior in other lists in the app should remain the same. There have not been any logic changes to the old behavior, but just make sure that is the case.

Screen.Recording.2024-05-05.at.4.39.48.AM.mov

Of course, the native behavior also remains the same:

Screen.Recording.2024-05-05.at.4.41.20.AM.mov

Copy link

render bot commented May 5, 2024

Copy link

github-actions bot commented May 5, 2024

Old size New size Diff
6.87 MB 6.87 MB 1.5 KB (0.02%)

@haileyok haileyok changed the title Further align List with FlatList, add contain mode to web list implementation Further align web List with FlatList, add contain mode to web list implementation May 5, 2024
@haileyok haileyok changed the base branch from main to adjust-on-start-end-reached-web May 5, 2024 08:28
@haileyok haileyok marked this pull request as ready for review May 5, 2024 08:30
Comment on lines 379 to 382
const observer = new IntersectionObserver(handleIntersection, {
root,
rootMargin: `${topMargin} 0px ${bottomMargin} 0px`,
})
Copy link
Contributor Author

@haileyok haileyok May 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default for IntersectionObserver is for root to be the viewport. This works fine whenever we are not using container mode, but when we are we want our intersections to be determined based on the container's scroll area, not the full window's. Will still default to null.

See: https://developer.mozilla.org/en-US/docs/Web/API/IntersectionObserver/root

Thanks mary! https://bsky.app/profile/mary.my.id/post/3krppk6yxbc2h

Base automatically changed from adjust-on-start-end-reached-web to main May 5, 2024 11:24
@haileyok haileyok requested a review from gaearon May 5, 2024 11:43
Comment on lines +89 to +136
const getScrollableNode = React.useCallback(() => {
if (containWeb) {
const element = nativeRef.current as HTMLDivElement | null
if (!element) return

return {
scrollWidth: element.scrollWidth,
scrollHeight: element.scrollHeight,
clientWidth: element.clientWidth,
clientHeight: element.clientHeight,
scrollY: element.scrollTop,
scrollX: element.scrollLeft,
scrollTo(options?: ScrollToOptions) {
element.scrollTo(options)
},
scrollBy(options: ScrollToOptions) {
element.scrollBy(options)
},
addEventListener(event: string, handler: any) {
element.addEventListener(event, handler)
},
removeEventListener(event: string, handler: any) {
element.removeEventListener(event, handler)
},
}
} else {
return {
scrollWidth: document.documentElement.scrollWidth,
scrollHeight: document.documentElement.scrollHeight,
clientWidth: window.innerWidth,
clientHeight: window.innerHeight,
scrollY: window.scrollY,
scrollX: window.scrollX,
scrollTo(options: ScrollToOptions) {
window.scrollTo(options)
},
scrollBy(options: ScrollToOptions) {
window.scrollBy(options)
},
addEventListener(event: string, handler: any) {
window.addEventListener(event, handler)
},
removeEventListener(event: string, handler: any) {
window.removeEventListener(event, handler)
},
}
}
}, [containWeb])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the main thing to pay attention to. It feels safer to create a a single source of truth on what a function does or a value returns rather than have a bunch of if (containWeb) {} else {} all over the component.

Copy link
Member

@estrattonbailey estrattonbailey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@haileyok haileyok merged commit bc07019 into main May 6, 2024
6 checks passed
@haileyok haileyok deleted the further-align-web-list-with-flatlist branch May 6, 2024 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants