-
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
…ectrum into s2-combobox-picker-virtualizer
the document didnt seem to be both updating its _minInvalidChildIndex nor updating its child indicies properly when the combobox list was filtered async (aka the collection got new items added and removed via insertBefore and/or removeChild. Additionally, we didnt see to ever call updateChildIndices on the Document other than the first time the collection loaded
// TODO: wull have to update this when multi section loading is implemented, will need to check if all items in a collection are loaders instead perhaps | ||
let hasLoadingSentinel = collection.size === 1 && collection.getItem(collection.getFirstKey()!)!.type === 'loader'; | ||
if (state.contentSize.area === 0 && !hasLoadingSentinel) { |
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.
This will change with https://github.com/adobe/react-spectrum/pull/8110/files#diff-6026043a183382ab1099d5cc5c47d7028cef57057d81b9dfc55acfba173e6d0f, can remove this logic then
@@ -103,7 +103,7 @@ export class BaseNode<T> { | |||
} | |||
|
|||
private invalidateChildIndices(child: ElementNode<T>): void { | |||
if (this._minInvalidChildIndex == null || child.index < this._minInvalidChildIndex.index) { | |||
if (this._minInvalidChildIndex == null || !this._minInvalidChildIndex.isConnected || child.index < this._minInvalidChildIndex.index) { |
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.
I found cases where the minInvalidChildIndex set on the Document was pointing to nodes that didn't exist in the collection any more so added this check so we'd always have an up to date one
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.
sounds maybe related to #8127?
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.
yeah a bit, definitely solves a part of that issue
// Is element.updateNode supposed to handle that (it currently assumes the index stored on the node is correct already). | ||
// At the moment, without this call to updateChildIndicies, filtering an async combobox doesn't actually update the index values of the | ||
// updated collection... | ||
this.updateChildIndices(); |
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.
This change along with the above were mainly to fix an issue I noticed where the collection indexes weren't updating when filtering the combobox. Above, we properly update the childIndices
of the Document's dirty nodes (aka the children's children) but not for the main children. Not sure if the call to element.updateNode
below is supposed to handle this but this felt the most correct
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.
shouldn't the document be in the list of dirtyNodes then and therefore get processed by the above loop?
also is the todo still relevant?
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.
The todo is part of the comment above, basically just noting that I'm not 100% sure about the change. From what I observed the document never marks itself as dirty when performing insertBefore
operations, only when calling appendChild
or if setFirstChild
gets hit in insertBefore
. Instead of this change, I could add this.ownerDocument.markDirty(this);
to BaseNode's insertBefore
and removeChild
but it felt safer to isolate it here to updateCollection
as a "end step" of sorts
// TODO: for some reason this tree renders empty if ran with the above test... | ||
// Even if the above test doesn't do anything within it, the below tree won't render with content until the above test | ||
// is fully commented out (aka even the it(...)) | ||
// It thinks that the contextSize is 0 and never updates | ||
it.skip('should not reserve room for the loader if isLoading is false', () => { |
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.
Not quite sure why this is happening... As stated, the below test only renders the listbox items if the above test is completely commented out. Even if the above test doesn't have any contents (aka just an empty it()
) this test still fails to render the ListBox content
@@ -372,7 +372,7 @@ export function usePress(props: PressHookProps): PressResult { | |||
if (isDisabled) { | |||
e.preventDefault(); | |||
} | |||
|
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.
could undo the changes in this file to cut down on changes/files touched
* @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.
would that be different than the ref already provided? a ref to the scrollable region seems like a good idea so that eventually people can use something like body element scrolling to drive the virtualizer
|
||
useLayoutEffect(() => { | ||
if (ref.current) { | ||
// 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 |
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.
seems fine, certainly more readable than separating it into some other effect and has the benefit of being queued in the same way as the rest of the IntersectionObserver updates
@@ -209,7 +209,11 @@ export function useScrollView(props: ScrollViewProps, ref: RefObject<HTMLElement | |||
// https://github.com/reactwg/react-18/discussions/102 | |||
// @ts-ignore | |||
if (typeof IS_REACT_ACT_ENVIRONMENT === 'boolean' ? IS_REACT_ACT_ENVIRONMENT : typeof jest !== 'undefined') { | |||
updateSize(fn => fn()); | |||
// This is so we update size in a separate render but within the same act. Needs to be setState instead of refs |
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.
why? is there some event we could hook into instead?
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.
This is to specifically fix the tests where the order in which things run (such as when the ref is connected to the ScrollView and when we perform rect measurements) vs how focus is moved by simulated clicks/presses from userEvent was causing odd behaviors (i.e. focus would move to the trigger button -> the dropdown -> back to the button specifically due to the order in which the dropdown becomes available/focusable + preventFocus
's logic thinking it should then restore focus to the button vs how it happens in the browser with the dropdown becoming available after that preventFocus
logic runs).
The only way to fix this was to make sure the size of the ScrollView updates in a separate render but within the same act, but ideally the high level flow should be that focus moves to the Picker trigger on click -> all press handling finishes -> collection is fully formed -> dropdown opens and focus moves into it.
|
||
return ( | ||
<> | ||
{/* TODO: Alway render the sentinel. Will need to figure out how best to position this in cases where the user is using flex + gap (this would introduce a gap even though it doesn't take room) */} |
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.
users will have to do it depending on their approach I think
<TD role="rowheader" {...rowHeaderProps} style={style}> | ||
{renderProps.children} | ||
</TD> | ||
{/* TODO weird structure? Renders a extra row but we hide via inert so maybe ok? */} |
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.
Going to be problematic for the work Yihui is doing to keyboard navigate to the the 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.
we could perhaps skip it via checking some attribute or something right?
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.
I do think we should use the sentinel's specifically for load more, and use renderEmptyState
for initial loading. That would possibly simplify some of the logic here.
@@ -103,7 +103,7 @@ export class BaseNode<T> { | |||
} | |||
|
|||
private invalidateChildIndices(child: ElementNode<T>): void { | |||
if (this._minInvalidChildIndex == null || child.index < this._minInvalidChildIndex.index) { | |||
if (this._minInvalidChildIndex == null || !this._minInvalidChildIndex.isConnected || child.index < this._minInvalidChildIndex.index) { |
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.
sounds maybe related to #8127?
// Is element.updateNode supposed to handle that (it currently assumes the index stored on the node is correct already). | ||
// At the moment, without this call to updateChildIndicies, filtering an async combobox doesn't actually update the index values of the | ||
// updated collection... | ||
this.updateChildIndices(); |
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.
shouldn't the document be in the list of dirtyNodes then and therefore get processed by the above loop?
also is the todo still relevant?
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.
why were all the styles copied?
let collection = this.virtualizer!.collection; | ||
// Make sure to set rows to 0 if we performing a first time load or are rendering the empty state so that Virtualizer | ||
// won't try to render its body | ||
let isEmptyOrLoading = collection?.size === 0 || (collection.size === 1 && collection.getItem(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.
if we are recommending people use renderEmptyState
for initial loading, do we still need this logic?
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 above, renderEmptyState
is used for initial loading but the LoadingSentinel is expected to always be included as a child
// Always add the loader sentinel if present in the collection so we can make sure it is never virtualized out. | ||
let lastNode = collection.getItem(collection.getLastKey()!); | ||
if (lastNode?.type === 'loader') { | ||
let rect = new Rect(horizontalSpacing, y, itemWidth, 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.
hard coded zero height? also this assumes the loader is always the last item in the collection
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.
For the CardView we don't have a spinner so I've set it to 0. Can definitely revisit if we expect multi section loading there or for a spinner to be used instead of Skeleton loading but for now I opted for the simpler change
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.
probably will need to be revisited for RAC users too. there might be a spinner there
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.
right good point, I'll add a todo so I don't forget
// 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.
could remove if initial loading state was handled in renderEmptyState
…ct-spectrum into loadmore_rac
## 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/utils/@react-aria/utils:UNSTABLE_useLoadMoreSentinel+UNSTABLE_useLoadMoreSentinel {
+ props: LoadMoreSentinelProps
+ ref: RefObject<HTMLElement | null>
+ returnVal: undefined
+} /@react-aria/utils:LoadMoreSentinelProps+LoadMoreSentinelProps {
+ collection: Collection<Node<unknown>>
+ 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 | null) => 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 | null) => 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'
} |
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