Skip to content

feat: Async loading support for S2 ComboBox/Picker #7938

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

Open
wants to merge 58 commits into
base: main
Choose a base branch
from
Open

Conversation

LFDanLu
Copy link
Member

@LFDanLu LFDanLu commented Mar 14, 2025

Closes

  • Adds loading support for S2 ComboBox/Picker and their RAC equivalents (and RAC ListBox too since that is what ComboBox and Picker uses
  • refactors useLoadMore to accept collections instead of items

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

Test S2 Combobox/Picker async loading stories and make sure it works as expected (aka infinite scrolling). Also test the RAC equivalents as well as S2 CardView/Table/other components that already had async loading support.

🧢 Your Project:

RSP

@rspbot
Copy link

rspbot commented Mar 15, 2025

@rspbot
Copy link

rspbot commented Mar 17, 2025

@rspbot
Copy link

rspbot commented Mar 18, 2025

@rspbot
Copy link

rspbot commented Mar 19, 2025

@LFDanLu LFDanLu changed the title feat: (WIP) Async loading support for S2 ComboBox and Picker feat: (WIP) Async loading support for S2 ComboBox/Picker and multilevel Tree loading Apr 8, 2025
@LFDanLu LFDanLu changed the title feat: (WIP) Async loading support for S2 ComboBox/Picker and multilevel Tree loading feat: Async loading support for S2 ComboBox/Picker Apr 9, 2025
@LFDanLu LFDanLu marked this pull request as ready for review April 9, 2025 21:12
@rspbot
Copy link

rspbot commented Apr 9, 2025

Copy link
Member Author

Choose a reason for hiding this comment

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

Make a new hook since util package version might not match up with pinned version of tableview/etc, which makes this a breaking change

@rspbot
Copy link

rspbot commented Apr 19, 2025

@rspbot
Copy link

rspbot commented Apr 22, 2025

LFDanLu added 3 commits April 22, 2025 17:17
…ed components

works for the most part but is problematic if you dont want the loadingRow to appear with performing initial load (aka loadingState = loading). Due to how useLoadMoreSentinel work, we need to reobserve the list when we go from loading to loadingMore, but is isLoading is true the layout willpreserve room for the loading row...
… load

also fixes RAC examples by properly applying a height to the tablebody if performing initial load
@rspbot
Copy link

rspbot commented Apr 24, 2025

paddingStart: 'edge-to-text'
});

export let listbox = style<{size: 'S' | 'M' | 'L' | 'XL'}>({
Copy link
Member Author

Choose a reason for hiding this comment

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

}

// TODO: not quite sure why typescript is complaining when I types this as T extends object
const ComboboxInner = forwardRef(function ComboboxInner(props: ComboBoxProps<any> & {isOpen: boolean}, ref: ForwardedRef<TextFieldRef | null>) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Mainly a reorg of code + adding loader logic/spinner rendering, for the most part most of this is the same as before

@rspbot
Copy link

rspbot commented Apr 24, 2025

@rspbot
Copy link

rspbot commented Apr 24, 2025

## API Changes

react-aria-components

/react-aria-components:UNSTABLE_TableLoadingIndicator

-UNSTABLE_TableLoadingIndicator <T extends {}> {
-  children?: ReactNode
-  className?: string
-  style?: CSSProperties
-}

/react-aria-components:UNSTABLE_GridListLoadingSentinel

+UNSTABLE_GridListLoadingSentinel <T extends {}> {
+  children?: ReactNode
+  className?: string
+  isLoading?: boolean
+  onLoadMore?: () => any
+  scrollOffset?: number = 1
+  style?: CSSProperties
+}

/react-aria-components:UNSTABLE_ListBoxLoadingSentinel

+UNSTABLE_ListBoxLoadingSentinel <T extends {}> {
+  children?: ReactNode
+  className?: string
+  isLoading?: boolean
+  onLoadMore?: () => any
+  scrollOffset?: number = 1
+  style?: CSSProperties
+}

/react-aria-components:UNSTABLE_TableLoadingSentinel

+UNSTABLE_TableLoadingSentinel <T extends {}> {
+  children?: ReactNode
+  className?: string
+  isLoading?: boolean
+  onLoadMore?: () => any
+  scrollOffset?: number = 1
+  style?: CSSProperties
+}

/react-aria-components:ListBoxLoadingSentinelProps

+ListBoxLoadingSentinelProps {
+  children?: ReactNode
+  className?: string
+  isLoading?: boolean
+  onLoadMore?: () => any
+  scrollOffset?: number = 1
+  style?: CSSProperties
+}

@react-aria/utils

/@react-aria/utils:UNSTABLE_useLoadMoreSentinel

+UNSTABLE_useLoadMoreSentinel {
+  props: LoadMoreSentinelProps
+  ref: RefObject<HTMLElement | null>
+  returnVal: undefined
+}

/@react-aria/utils:LoadMoreSentinelProps

+LoadMoreSentinelProps {
+  isLoading?: boolean
+  onLoadMore?: () => any
+  scrollOffset?: number = 1
+}

@react-spectrum/s2

/@react-spectrum/s2:ComboBox

 ComboBox <T extends {}> {
   UNSAFE_className?: UnsafeClassName
   UNSAFE_style?: CSSProperties
   align?: 'start' | 'end' = 'start'
   allowsCustomValue?: boolean
   aria-describedby?: string
   aria-details?: string
   aria-label?: string
   aria-labelledby?: string
   autoFocus?: boolean
   children: ReactNode | ({}) => ReactNode
   contextualHelp?: ReactNode
   defaultInputValue?: string
   defaultItems?: Iterable<T>
   defaultSelectedKey?: Key
   description?: ReactNode
   direction?: 'bottom' | 'top' = 'bottom'
   disabledKeys?: Iterable<Key>
   errorMessage?: ReactNode | (ValidationResult) => ReactNode
   formValue?: 'text' | 'key' = 'key'
   id?: string
   inputValue?: string
   isDisabled?: boolean
   isInvalid?: boolean
   isReadOnly?: boolean
   isRequired?: boolean
   items?: Iterable<T>
   label?: ReactNode
   labelAlign?: Alignment = 'start'
   labelPosition?: LabelPosition = 'top'
+  loadingState?: LoadingState
   menuTrigger?: MenuTriggerAction = 'input'
   menuWidth?: number
   name?: string
   necessityIndicator?: NecessityIndicator = 'icon'
   onBlur?: (FocusEvent<HTMLInputElement>) => void
   onFocus?: (FocusEvent<HTMLInputElement>) => void
   onFocusChange?: (boolean) => void
   onInputChange?: (string) => void
   onKeyDown?: (KeyboardEvent) => void
   onKeyUp?: (KeyboardEvent) => void
+  onLoadMore?: () => any
   onOpenChange?: (boolean, MenuTriggerAction) => void
   onSelectionChange?: (Key | null) => void
   selectedKey?: Key | null
   shouldFlip?: boolean = true
   size?: 'S' | 'M' | 'L' | 'XL' = 'M'
   slot?: string | null
   styles?: StylesProp
   validate?: (ComboBoxValidationValue) => ValidationError | boolean | null | undefined
   validationBehavior?: 'native' | 'aria' = 'native'
 }

/@react-spectrum/s2:Picker

 Picker <T extends {}> {
   UNSAFE_className?: UnsafeClassName
   UNSAFE_style?: CSSProperties
   align?: 'start' | 'end' = 'start'
   aria-describedby?: string
   aria-details?: string
   aria-label?: string
   aria-labelledby?: string
   autoComplete?: string
   autoFocus?: boolean
   children: ReactNode | ({}) => ReactNode
   contextualHelp?: ReactNode
   defaultOpen?: boolean
   defaultSelectedKey?: Key
   description?: ReactNode
   direction?: 'bottom' | 'top' = 'bottom'
   disabledKeys?: Iterable<Key>
   errorMessage?: ReactNode | (ValidationResult) => ReactNode
   excludeFromTabOrder?: boolean
   id?: string
   isDisabled?: boolean
   isInvalid?: boolean
+  isLoading?: boolean
   isOpen?: boolean
   isRequired?: boolean
   items?: Iterable<T>
   label?: ReactNode
   labelAlign?: Alignment = 'start'
   labelPosition?: LabelPosition = 'top'
   menuWidth?: number
   name?: string
   necessityIndicator?: NecessityIndicator = 'icon'
   onBlur?: (FocusEvent<Target>) => void
   onFocus?: (FocusEvent<Target>) => void
   onFocusChange?: (boolean) => void
   onKeyDown?: (KeyboardEvent) => void
   onKeyUp?: (KeyboardEvent) => void
+  onLoadMore?: () => any
   onOpenChange?: (boolean) => void
   onSelectionChange?: (Key) => void
   placeholder?: string = 'Select an item' (localized)
   selectedKey?: Key | null
   size?: 'S' | 'M' | 'L' | 'XL' = 'M'
   slot?: string | null
   styles?: StylesProp
   validate?: (Key) => ValidationError | boolean | null | undefined
   validationBehavior?: 'native' | 'aria' = 'native'
 }

/@react-spectrum/s2:ComboBoxProps

 ComboBoxProps <T extends {}> {
   UNSAFE_className?: UnsafeClassName
   UNSAFE_style?: CSSProperties
   align?: 'start' | 'end' = 'start'
   allowsCustomValue?: boolean
   aria-describedby?: string
   aria-details?: string
   aria-label?: string
   aria-labelledby?: string
   autoFocus?: boolean
   children: ReactNode | ({}) => ReactNode
   contextualHelp?: ReactNode
   defaultInputValue?: string
   defaultItems?: Iterable<T>
   defaultSelectedKey?: Key
   description?: ReactNode
   direction?: 'bottom' | 'top' = 'bottom'
   disabledKeys?: Iterable<Key>
   errorMessage?: ReactNode | (ValidationResult) => ReactNode
   formValue?: 'text' | 'key' = 'key'
   id?: string
   inputValue?: string
   isDisabled?: boolean
   isInvalid?: boolean
   isReadOnly?: boolean
   isRequired?: boolean
   items?: Iterable<T>
   label?: ReactNode
   labelAlign?: Alignment = 'start'
   labelPosition?: LabelPosition = 'top'
+  loadingState?: LoadingState
   menuTrigger?: MenuTriggerAction = 'input'
   menuWidth?: number
   name?: string
   necessityIndicator?: NecessityIndicator = 'icon'
   onBlur?: (FocusEvent<HTMLInputElement>) => void
   onFocus?: (FocusEvent<HTMLInputElement>) => void
   onFocusChange?: (boolean) => void
   onInputChange?: (string) => void
   onKeyDown?: (KeyboardEvent) => void
   onKeyUp?: (KeyboardEvent) => void
+  onLoadMore?: () => any
   onOpenChange?: (boolean, MenuTriggerAction) => void
   onSelectionChange?: (Key | null) => void
   selectedKey?: Key | null
   shouldFlip?: boolean = true
   size?: 'S' | 'M' | 'L' | 'XL' = 'M'
   slot?: string | null
   styles?: StylesProp
   validate?: (ComboBoxValidationValue) => ValidationError | boolean | null | undefined
   validationBehavior?: 'native' | 'aria' = 'native'
 }

/@react-spectrum/s2:PickerProps

 PickerProps <T extends {}> {
   UNSAFE_className?: UnsafeClassName
   UNSAFE_style?: CSSProperties
   align?: 'start' | 'end' = 'start'
   aria-describedby?: string
   aria-details?: string
   aria-label?: string
   aria-labelledby?: string
   autoComplete?: string
   autoFocus?: boolean
   children: ReactNode | ({}) => ReactNode
   contextualHelp?: ReactNode
   defaultOpen?: boolean
   defaultSelectedKey?: Key
   description?: ReactNode
   direction?: 'bottom' | 'top' = 'bottom'
   disabledKeys?: Iterable<Key>
   errorMessage?: ReactNode | (ValidationResult) => ReactNode
   excludeFromTabOrder?: boolean
   id?: string
   isDisabled?: boolean
   isInvalid?: boolean
+  isLoading?: boolean
   isOpen?: boolean
   isRequired?: boolean
   items?: Iterable<T>
   label?: ReactNode
   labelAlign?: Alignment = 'start'
   labelPosition?: LabelPosition = 'top'
   menuWidth?: number
   name?: string
   necessityIndicator?: NecessityIndicator = 'icon'
   onBlur?: (FocusEvent<Target>) => void
   onFocus?: (FocusEvent<Target>) => void
   onFocusChange?: (boolean) => void
   onKeyDown?: (KeyboardEvent) => void
   onKeyUp?: (KeyboardEvent) => void
+  onLoadMore?: () => any
   onOpenChange?: (boolean) => void
   onSelectionChange?: (Key) => void
   placeholder?: string = 'Select an item' (localized)
   selectedKey?: Key | null
   size?: 'S' | 'M' | 'L' | 'XL' = 'M'
   slot?: string | null
   styles?: StylesProp
   validate?: (Key) => ValidationError | boolean | null | undefined
   validationBehavior?: 'native' | 'aria' = 'native'
 }

* @default 1
*/
scrollOffset?: number
// TODO: Maybe include a scrollRef option so the user can provide the scrollParent to compare against instead of having us look it up
Copy link
Member Author

Choose a reason for hiding this comment

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

leaning towards not adding it for now, the hook is unstable as of now so we could always add it later. Open to opinions if anyone feels strongly about the approach

Comment on lines +48 to +53
// 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
Copy link
Member Author

Choose a reason for hiding this comment

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

A bit unfortunate that fixing this involved layout level changes. Also a bit unfortunate that we can't just create single instance of a IntersectionObserver upfront, but we need to do this setup/teardown every time isLoading changes so that we can properly trigger the loadMore if the sentinel remains within the rootMargin after the items are loaded.

@@ -6,6 +6,7 @@
"actionbar.selectedAll": "تم تحديد الكل",
"breadcrumbs.more": "المزيد من العناصر",
"button.pending": "قيد الانتظار",
"combobox.noResults": "لا توجد نتائج",
Copy link
Member Author

Choose a reason for hiding this comment

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

From S1

Comment on lines +561 to +565
// 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
isLoading={loadingState === 'loadingMore'}
Copy link
Member Author

Choose a reason for hiding this comment

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

An interesting facet about how useLoadMoreSentinel works. Prior to some additional changes in layout, virtualized TableView ran into issues triggering additional loadMore calls when it was passing isLoading={loadingState === 'loadingMore'} to its own sentinel. This is because the sentinel never got a prop change before and after the initial load finished because the loadingState at that point was loading and thus we never caused a second shouldLoadMore check to happen in useLoadMoreSentinel

Comment on lines +697 to +700
// TODO: can we get rid of this and instead handle padding else where other than the layout options? Kinda gross that we need to do this check.
// Perhaps can consider only applying padding if the collection actually has content since the renderEmptyState content is outside the virtualizer
// Otherwise could consider moving renderEmptyState into the collection...
padding: isEmptyOrLoading ? 0 : 8,
Copy link
Member Author

Choose a reason for hiding this comment

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

we actually don't want any extra padding (or have the virtualizer div render anything really) so that the "No results"/isLoading spinner dictate the height of the the popover. This check is a bit gross but not really sure there is a good way to do this otherwise.

Comment on lines +199 to 203
// TODO: we may want to adjust RAC layouts to do something simlar? Current users of RAC table will probably run into something similar
let isEmptyOrLoading = children?.length === 0 || (children?.length === 1 && children[0].layoutInfo.type === 'loader');
if (isEmptyOrLoading) {
layoutInfo.rect.width = this.virtualizer!.visibleRect.width - 80;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

One of the several places that I had to change from our prior assumption of "empty = collection doesn't have children". Quite specific to a virtualized table but might be useful to document or include by default?

Comment on lines +382 to +384
// TODO: still is offset strangely if loadingMore when there aren't any items in the table, see http://localhost:6006/?path=/story/tableview--empty-state&args=loadingState:loadingMore
// 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.
Copy link
Member Author

Choose a reason for hiding this comment

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

Specifically, this is a case where you have a empty table and pass isLoadingMore as your loading state. The loadingMore spinner row won't be built by the layout since the layout thinks you are performing initial load so in the story linked the body is actually just blank. IMO user error but otherwise we need more information in the layout to differentiate the two cases

Comment on lines +345 to +348
// 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;
Copy link
Member Author

Choose a reason for hiding this comment

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

As mentioned previously, we don't want to render the loadingMore progress spinner row when performing initial load otherwise we get two spinners and/or extra room rendered above the isLoading spinner

Comment on lines +197 to +198
// TODO: What do we think about this check? Ideally we could just query the collection and see if ALL node are loaders and thus have it return that it is empty
let isEmpty = state.collection.size === 0 || (state.collection.size === 1 && state.collection.getItem(state.collection.getFirstKey()!)?.type === 'loader');
Copy link
Member Author

Choose a reason for hiding this comment

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

Will need to update this logic (and other areas where I've made the assumption that the loader is the last element in the collection) when we move to supporting tree multi section loading

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

Successfully merging this pull request may close these issues.

3 participants