refactor to use collection instead of isLoading in useLoadMoreSentinel

this allows us to only have one prop in the sentinel to control visiblility of the loader while still calling loadMore when the collection changes. Also gives the added benefit of causing load more to happen if items are deleted from the collection

Author: LFDanLu

packages/@react-aria/utils/src/useLoadMoreSentinel.ts

@@ -10,13 +10,14 @@
  * governing permissions and limitations under the License.
  */
 
-import type {AsyncLoadable} from '@react-types/shared';
+import type {AsyncLoadable, Collection, Node} from '@react-types/shared';
 import {getScrollParent} from './getScrollParent';
 import {RefObject, useRef} from 'react';
 import {useEffectEvent} from './useEffectEvent';
 import {useLayoutEffect} from './useLayoutEffect';
 
-export interface LoadMoreSentinelProps extends AsyncLoadable {
+export interface LoadMoreSentinelProps extends Omit<AsyncLoadable, 'isLoading'> {
+  collection: Collection<Node<unknown>>,
   /**
    * The amount of offset from the bottom of your scrollable region that should trigger load more.
    * Uses a percentage value relative to the scroll body's client height. Load more is then triggered
@@ -29,28 +30,27 @@ export interface LoadMoreSentinelProps extends AsyncLoadable {
 }
 
 export function UNSTABLE_useLoadMoreSentinel(props: LoadMoreSentinelProps, ref: RefObject<HTMLElement | null>): void {
-  let {isLoading, onLoadMore, scrollOffset = 1} = props;
+  let {collection, onLoadMore, scrollOffset = 1} = props;
 
   let sentinelObserver = useRef<IntersectionObserver>(null);
 
   let triggerLoadMore = useEffectEvent((entries: IntersectionObserverEntry[]) => {
     // Use "isIntersecting" over an equality check of 0 since it seems like there is cases where
     // a intersection ratio of 0 can be reported when isIntersecting is actually true
     for (let entry of entries) {
-      if (entry.isIntersecting && !isLoading && onLoadMore) {
+      // Note that this will be called if the collection changes, even if onLoadMore was already called and is being processed.
+      // Up to user discretion as to how to handle these multiple onLoadMore calls
+      if (entry.isIntersecting && onLoadMore) {
         onLoadMore();
       }
     }
   });
 
   useLayoutEffect(() => {
     if (ref.current) {
-      // TODO: problem with S2's Table loading spinner. Now that we use the isLoading provided to the sentinel in the layout to adjust the height of the loader,
-      // we are getting space reserved for the loadMore spinner when doing initial loading and rendering empty state at the same time. We can somewhat fix this by providing isLoading={loadingState === 'loadingMore'}
-      // which will mean the layout won't reserve space for the loader for initial loads, but that breaks the load more behavior (specifically, auto load more to fill scrollOffset. Scroll to load more seems broken to after initial load).
-      // We need to tear down and set up a new IntersectionObserver to force a check if the sentinel is "in view", see  https://codesandbox.io/p/sandbox/magical-swanson-dhgp89?file=%2Fsrc%2FApp.js%3A21%2C21
-      // I've actually fixed this via a ListLayout change (TableLayout extends this) where I check "collection?.size === 0 || (collection.size === 1 && collection.getItem(collection.getFirstKey()!)!.type === 'loader')"
-      // as well as isLoading, but it feels pretty opinionated/implementation specifc
+      // Tear down and set up a new IntersectionObserver when the collection changes so that we can properly trigger additional loadMores if there is room for more items
+      // Need to do this tear down and set up since using a large rootMargin will mean the observer's callback isn't called even when scrolling the item into view beause its visibility hasn't actually changed
+      // https://codesandbox.io/p/sandbox/magical-swanson-dhgp89?file=%2Fsrc%2FApp.js%3A21%2C21
       sentinelObserver.current = new IntersectionObserver(triggerLoadMore, {root: getScrollParent(ref?.current) as HTMLElement, rootMargin: `0px ${100 * scrollOffset}% ${100 * scrollOffset}% ${100 * scrollOffset}%`});
       sentinelObserver.current.observe(ref.current);
     }
@@ -60,5 +60,5 @@ export function UNSTABLE_useLoadMoreSentinel(props: LoadMoreSentinelProps, ref:
         sentinelObserver.current.disconnect();
       }
     };
-  }, [isLoading, triggerLoadMore, ref, scrollOffset]);
+  }, [collection, triggerLoadMore, ref, scrollOffset]);
 }

packages/@react-spectrum/s2/src/CardView.tsx

@@ -245,7 +245,6 @@ export const CardView = /*#__PURE__*/ (forwardRef as forwardRefType)(function Ca
   let renderer;
   let cardLoadingSentinel = (
     <UNSTABLE_GridListLoadingSentinel
-      isLoading={props.loadingState === 'loading' || props.loadingState === 'filtering' || props.loadingState === 'loadingMore'}
       onLoadMore={onLoadMore} />
   );
 

packages/@react-spectrum/s2/src/ComboBox.tsx

@@ -558,21 +558,16 @@ const ComboboxInner = forwardRef(function ComboboxInner(props: ComboBoxProps<any
   let renderer;
   let listBoxLoadingCircle = (
     <UNSTABLE_ListBoxLoadingSentinel
-      // TODO: we only want the listbox loading sentinel row to have a height when performing a loadingMore
-      // Unlike table, this works because the listbox's initial load isn't triggerd by the sentintel and thus the first
-      // observation occurs when we've already loaded our first set of items rather than starting from empty, therefor flipping
-      // isLoading here from true to false and triggering the creation of a new InterserctionObserver
+      // Only show the spinner in the list when loading more
       isLoading={loadingState === 'loadingMore'}
       onLoadMore={onLoadMore}
       className={loadingWrapperStyles}>
-      {loadingState === 'loadingMore' && (
-        <ProgressCircle
-          isIndeterminate
-          size="S"
-          styles={progressCircleStyles({size})}
-          // Same loading string as table
-          aria-label={stringFormatter.format('table.loadingMore')} />
-      )}
+      <ProgressCircle
+        isIndeterminate
+        size="S"
+        styles={progressCircleStyles({size})}
+        // Same loading string as table
+        aria-label={stringFormatter.format('table.loadingMore')} />
     </UNSTABLE_ListBoxLoadingSentinel>
   );
 

packages/@react-spectrum/s2/src/TableView.tsx

@@ -383,14 +383,12 @@ export const TableBody = /*#__PURE__*/ (forwardRef as forwardRefType)(function T
   // This is because we don't distinguish between loadingMore and loading in the layout, resulting in a different rect being used to build the body. Perhaps can be considered as a user error
   // if they pass loadingMore without having any other items in the table. Alternatively, could update the layout so it knows the current loading state.
   let loadMoreSpinner = (
-    <UNSTABLE_TableLoadingSentinel isLoading={isLoading} onLoadMore={onLoadMore} className={style({height: 'full', width: 'full'})}>
-      {loadingState === 'loadingMore' && (
-        <div className={centeredWrapper}>
-          <ProgressCircle
-            isIndeterminate
-            aria-label={stringFormatter.format('table.loadingMore')} />
-        </div>
-      )}
+    <UNSTABLE_TableLoadingSentinel isLoading={loadingState === 'loadingMore'} onLoadMore={onLoadMore} className={style({height: 'full', width: 'full'})}>
+      <div className={centeredWrapper}>
+        <ProgressCircle
+          isIndeterminate
+          aria-label={stringFormatter.format('table.loadingMore')} />
+      </div>
     </UNSTABLE_TableLoadingSentinel>
   );
 

packages/@react-spectrum/s2/stories/ComboBox.stories.tsx

@@ -223,7 +223,7 @@ const AsyncComboBox = (args: any) => {
       }
 
       // Slow down load so progress circle can appear
-      await new Promise(resolve => setTimeout(resolve, 2000));
+      await new Promise(resolve => setTimeout(resolve, args.delay));
       let res = await fetch(cursor || `https://swapi.py4e.com/api/people/?search=${filterText}`, {signal});
       let json = await res.json();
 
@@ -251,7 +251,8 @@ export const AsyncComboBoxStory  = {
   render: AsyncComboBox,
   args: {
     ...Example.args,
-    label: 'Star Wars Character Lookup'
+    label: 'Star Wars Character Lookup',
+    delay: 50
   },
   name: 'Async loading combobox',
   parameters: {

packages/@react-spectrum/s2/stories/Picker.stories.tsx

@@ -240,7 +240,7 @@ const AsyncPicker = (args: any) => {
       }
 
       // Slow down load so progress circle can appear
-      await new Promise(resolve => setTimeout(resolve, 2000));
+      await new Promise(resolve => setTimeout(resolve, args.delay));
       let res = await fetch(cursor || 'https://swapi.py4e.com/api/people/?search=', {signal});
       let json = await res.json();
       return {
@@ -260,7 +260,8 @@ const AsyncPicker = (args: any) => {
 export const AsyncPickerStory  = {
   render: AsyncPicker,
   args: {
-    ...Example.args
+    ...Example.args,
+    delay: 50
   },
   name: 'Async loading picker',
   parameters: {

packages/@react-spectrum/s2/stories/TableView.stories.tsx

@@ -428,7 +428,7 @@ const OnLoadMoreTable = (args: any) => {
       }
 
       // Slow down load so progress circle can appear
-      await new Promise(resolve => setTimeout(resolve, 2000));
+      await new Promise(resolve => setTimeout(resolve, args.delay));
       let res = await fetch(cursor || 'https://swapi.py4e.com/api/people/?search=', {signal});
       let json = await res.json();
       return {
@@ -464,7 +464,8 @@ const OnLoadMoreTable = (args: any) => {
 export const OnLoadMoreTableStory  = {
   render: OnLoadMoreTable,
   args: {
-    ...Example.args
+    ...Example.args,
+    delay: 50
   },
   name: 'async loading table'
 };

packages/@react-spectrum/s2/test/Combobox.test.tsx

@@ -49,8 +49,7 @@ describe('Combobox', () => {
     expect(within(comboboxTester.listbox!).getByTestId('loadMoreSentinel')).toBeInTheDocument();
   });
 
-  // TODO: will need to update these tests when I replace isLoading with colection in useLoadMoreSentinel
-  it('should only call loadMore if loading is false', async () => {
+  it('should only call loadMore whenever intersection is detected', async () => {
     let onLoadMore = jest.fn();
     let observe = jest.fn();
     let observer = setupIntersectionObserverMock({
@@ -92,6 +91,8 @@ describe('Combobox', () => {
 
     act(() => {observer.instance.triggerCallback([{isIntersecting: true}]);});
     act(() => {jest.runAllTimers();});
-    expect(onLoadMore).toHaveBeenCalledTimes(1);
+    // Note that if this was using useAsyncList, we'd be shielded from extranous onLoadMore calls but
+    // we want to leave that to user discretion
+    expect(onLoadMore).toHaveBeenCalledTimes(2);
   });
 });

packages/@react-stately/layout/src/ListLayout.ts

@@ -337,15 +337,12 @@ export class ListLayout<T, O extends ListLayoutOptions = ListLayoutOptions> exte
   }
 
   protected buildLoader(node: Node<T>, x: number, y: number): LayoutNode {
-    let collection = this.virtualizer!.collection;
-    let isEmptyOrLoading = collection?.size === 0 || (collection.size === 1 && collection.getItem(collection.getFirstKey()!)!.type === 'loader');
     let rect = new Rect(x, y, this.padding, 0);
     let layoutInfo = new LayoutInfo('loader', node.key, rect);
     rect.width = this.virtualizer!.contentSize.width - this.padding - x;
-    // TODO: Kinda gross but we also have to differentiate between isLoading and isLoadingMore so that we dont'reserve room
-    // for the loadMore loader row when we are performing initial load. Is this too opinionated? Note that users creating their own layouts
-    // may need to perform similar logic
-    rect.height = node.props.isLoading && !isEmptyOrLoading ? this.loaderHeight ?? this.rowHeight ?? this.estimatedRowHeight ?? DEFAULT_HEIGHT : 0;
+    // Note that if the user provides isLoading to their sentinel during a case where they only want to render the emptyState, this will reserve
+    // room for the loader alongside rendering the emptyState
+    rect.height = node.props.isLoading ? this.loaderHeight ?? this.rowHeight ?? this.estimatedRowHeight ?? DEFAULT_HEIGHT : 0;
 
     return {
       layoutInfo,

packages/react-aria-components/src/GridList.tsx

@@ -497,23 +497,29 @@ function RootDropIndicator() {
   );
 }
 
-export interface GridListLoadingIndicatorProps extends LoadMoreSentinelProps, StyleProps {
-  children?: ReactNode
+export interface GridListLoadingSentinelProps extends Omit<LoadMoreSentinelProps, 'collection'>, StyleProps {
+  /**
+   * The load more spinner to render when loading additional items.
+   */
+  children?: ReactNode,
+  /**
+   * Whether or not the loading spinner should be rendered or not.
+   */
+  isLoading?: boolean
 }
 
-export const UNSTABLE_GridListLoadingSentinel = createLeafComponent('loader', function GridListLoadingIndicator<T extends object>(props: GridListLoadingIndicatorProps, ref: ForwardedRef<HTMLDivElement>, item: Node<T>) {
+export const UNSTABLE_GridListLoadingSentinel = createLeafComponent('loader', function GridListLoadingIndicator<T extends object>(props: GridListLoadingSentinelProps, ref: ForwardedRef<HTMLDivElement>, item: Node<T>) {
   let state = useContext(ListStateContext)!;
   let {isVirtualized} = useContext(CollectionRendererContext);
   let {isLoading, onLoadMore, scrollOffset, ...otherProps} = props;
 
   let sentinelRef = useRef(null);
   let memoedLoadMoreProps = useMemo(() => ({
-    isLoading,
     onLoadMore,
     collection: state?.collection,
     sentinelRef,
     scrollOffset
-  }), [isLoading, onLoadMore, scrollOffset, state?.collection]);
+  }), [onLoadMore, scrollOffset, state?.collection]);
   UNSTABLE_useLoadMoreSentinel(memoedLoadMoreProps, sentinelRef);
 
   let renderProps = useRenderProps({
@@ -531,7 +537,7 @@ export const UNSTABLE_GridListLoadingSentinel = createLeafComponent('loader', fu
       <div style={{position: 'relative', width: 0, height: 0}} inert={inertValue(true)} >
         <div data-testid="loadMoreSentinel" ref={sentinelRef} style={{position: 'absolute', height: 1, width: 1}} />
       </div>
-      {isLoading && state.collection.size > 1 && renderProps.children && (
+      {isLoading && renderProps.children && (
         <div
           role="row"
           aria-rowindex={isVirtualized ? item.index + 1 : undefined}