-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
base: main
Are you sure you want to change the base?
Conversation
…and data-attributes
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.
Make a new hook since util package version might not match up with pinned version of tableview/etc, which makes this a breaking change
…ct-spectrum into loadmore_rac
…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
paddingStart: 'edge-to-text' | ||
}); | ||
|
||
export let listbox = style<{size: 'S' | 'M' | 'L' | 'XL'}>({ |
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.
Here to LOADER_ROW_HEIGHTS is from https://github.com/adobe/react-spectrum/pull/8110/files
} | ||
|
||
// 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>) { |
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.
Mainly a reorg of code + adding loader logic/spinner rendering, for the most part most of this is the same as before
was only needed when rendering the loader after the virutalizer div
## 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 |
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.
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
// 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 |
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.
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": "لا توجد نتائج", |
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 S1
// 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'} |
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.
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
// 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, |
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.
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.
// 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; | ||
} |
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.
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?
// 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. |
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.
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
// 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; |
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.
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
// 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'); |
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.
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
Closes
✅ Pull Request Checklist:
📝 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