feat: (React Aria) Implement filtering on a per CollectionNode basis #8641
Conversation
| let newCollection = new BaseCollection<T>(); | ||
| // This tracks the absolute last node we've visited in the collection when filtering, used for setting up the filteredCollection's lastKey and | ||
| // for updating the next/prevKey for every non-filtered node. | ||
| let lastNode: Mutable<CollectionNode<T>> | null = null; | ||
|
|
||
| for (let node of this) { | ||
| if (node.type === 'section' && node.hasChildNodes) { | ||
| let clonedSection: Mutable<CollectionNode<T>> = (node as CollectionNode<T>).clone(); | ||
| let lastChildInSection: Mutable<CollectionNode<T>> | null = null; | ||
| for (let child of this.getChildren(node.key)) { | ||
| if (shouldKeepNode(child, filterFn, this, newCollection)) { | ||
| let clonedChild: Mutable<CollectionNode<T>> = (child as CollectionNode<T>).clone(); | ||
| // eslint-disable-next-line max-depth | ||
| if (lastChildInSection == null) { | ||
| clonedSection.firstChildKey = clonedChild.key; | ||
| } | ||
|
|
||
| // eslint-disable-next-line max-depth | ||
| if (newCollection.firstKey == null) { | ||
| newCollection.firstKey = clonedSection.key; | ||
| } | ||
|
|
||
| // eslint-disable-next-line max-depth | ||
| if (lastChildInSection && lastChildInSection.parentKey === clonedChild.parentKey) { | ||
| lastChildInSection.nextKey = clonedChild.key; | ||
| clonedChild.prevKey = lastChildInSection.key; | ||
| } else { | ||
| clonedChild.prevKey = null; | ||
| } | ||
|
|
||
| clonedChild.nextKey = null; | ||
| newCollection.addNode(clonedChild); | ||
| lastChildInSection = clonedChild; | ||
| } | ||
| } | ||
| let [firstKey, lastKey] = filterChildren(this, newCollection, this.firstKey, filterFn); | ||
| newCollection.firstKey = firstKey; | ||
| newCollection.lastKey = lastKey; | ||
| return newCollection; | ||
| } | ||
| } |
There was a problem hiding this comment.
I was wondering whether we could introduce something like a parent prop here to track relationship between the filtered outcome and the original collection. Currently, filtered collections generate a different data-collection id every time the filter changes.
There was a problem hiding this comment.
I see, I'll play around with it a bit. Wonder if the data-collection id can just come from the collection directly
There was a problem hiding this comment.
Surely, although this would mean a merge of the id would likely trigger a rerender in the builder instead of the collection inner component. Not sure whether it was intentional to hook up useCollectionId with useId instead of useSSRId anyways?
There was a problem hiding this comment.
I thought the rerender from merging ids only happened for id and not for for say the data-collectionId but its been a while since I've dug through that code. As for the usage of useId in useCollectionId, is the concern around the rerendering? If so, I think its fine, just used to get a unique value but shouldn't be affected by the mergeProps id rerendering behavior I think as mentioned before
There was a problem hiding this comment.
No, you are good. I meant an explicit merge through mergeIds 👍 Or it happens someone for some reason decides to map data-collectionid to an id prop. Just wanted to make sure we are aware of what could happen, since this may lead to hard to debug issues real quick.
There was a problem hiding this comment.
Just a heads up, but the team discussed this a bit and would like to hold off on adding it until we discuss all the requirements/needs for these collection ids (and how it meshes with your other PRs) in your Carousel RFC
There was a problem hiding this comment.
got it, been working on the rfc, but its a lot of work. i expect it to land early next week 🙏
There was a problem hiding this comment.
sounds good, and thank you so much for going the extra mile!
|
Build successful! 🎉 |
…wrapped collection is filterable
…g for CollectionNodeClass
LFDanLu
changed the title
|
Build successful! 🎉 |
|
Build successful! 🎉 |
| }; | ||
|
|
||
| return ( | ||
| <Autocomplete filter={filter}> |
There was a problem hiding this comment.
for instance, is this picking things up correctly? I would've thought that you'd need to specify the <Autocomplete<{name: string, isSection?: boolean, children?: ReactNode}> to match the types of items
I don't think Autocomplete will be able to infer this, so I would expect the generic to be required. Then filter can be typed as well.
There was a problem hiding this comment.
I added a example with the proper MenuNode types. Here is a quick screenshot of what happens if you define the generic of the Autocomplete as one node, and the filter as a different node:
The onus is still on the user to type their filter function separately from the Autocomplete of course, but once that is done TS can make sure the provided Autocomplete generic matches the filter definition
|
Build successful! 🎉 |
| let NodeClass = function (key: Key) { | ||
| return new CollectionNode(type, key); | ||
| } as any; | ||
| NodeClass.type = type; | ||
| return NodeClass; |
There was a problem hiding this comment.
| let NodeClass = function (key: Key) { | |
| return new CollectionNode(type, key); | |
| } as any; | |
| NodeClass.type = type; | |
| return NodeClass; | |
| let NodeClass = class extends CollectionNode<any> { | |
| static readonly type = type; | |
| constructor(key: Key) { | |
| super(type, key); | |
| } | |
| }; | |
| return NodeClass; |
I think this is a little clearer as to what is going on
There was a problem hiding this comment.
you could memo the creation of the NodeClass as well if you wanted, then all the instances would be match
| static readonly type = 'item'; | ||
|
|
||
| constructor(key: Key) { | ||
| super(ItemNode.type, key); |
There was a problem hiding this comment.
Could the base CollectionNode class handle this? It could read this.constructor.type
There was a problem hiding this comment.
made the change in #8695 for convience
|
|
||
| filter(collection: BaseCollection<T>, newCollection: BaseCollection<T>, filterFn: FilterFn<T>): ItemNode<T> | null { | ||
| if (filterFn(this.textValue, this)) { | ||
| return this.clone(); |
There was a problem hiding this comment.
Do the nodes always need to be cloned? Might be faster to lazily clone only when needed (e.g. when updating prevKey/nextKey.
There was a problem hiding this comment.
hm, tried this and it broke filtering, digging
| @@ -60,7 +60,7 @@ export {ProgressBar, ProgressBarContext} from './ProgressBar'; | |||
| export {RadioGroup, Radio, RadioGroupContext, RadioContext, RadioGroupStateContext} from './RadioGroup'; | |||
| export {SearchField, SearchFieldContext} from './SearchField'; | |||
| export {Select, SelectValue, SelectContext, SelectValueContext, SelectStateContext} from './Select'; | |||
| export {Separator, SeparatorContext} from './Separator'; | |||
| export {Separator, SeparatorContext, SeparatorNode} from './Separator'; | |||
There was a problem hiding this comment.
I think we should just copy it into S2. Don't want to export these yet.
| @@ -305,10 +305,12 @@ function ListBoxSectionInner<T extends object>(props: ListBoxSectionProps<T>, re | |||
| ); | |||
| } | |||
|
|
|||
| export class ListBoxSectionNode<T> extends SectionNode<T> {} | |||
| @@ -513,7 +523,16 @@ export interface GridListLoadMoreItemProps extends Omit<LoadMoreSentinelProps, ' | |||
| isLoading?: boolean | |||
| } | |||
|
|
|||
| export const GridListLoadMoreItem = createLeafComponent('loader', function GridListLoadingIndicator(props: GridListLoadMoreItemProps, ref: ForwardedRef<HTMLDivElement>, item: Node<object>) { | |||
| // TODO: maybe make a general loader node | |||
| @@ -160,6 +161,7 @@ class TableCollection<T> extends BaseCollection<T> implements ITableCollection<T | |||
| collection.rowHeaderColumnKeys = this.rowHeaderColumnKeys; | |||
| collection.head = this.head; | |||
| collection.body = this.body; | |||
| collection.updateColumns = this.updateColumns; | |||
| @@ -190,6 +192,12 @@ class TableCollection<T> extends BaseCollection<T> implements ITableCollection<T | |||
|
|
|||
| return text.join(' '); | |||
| } | |||
|
|
|||
| filter(filterFn: (textValue: string, node: Node<T>) => boolean): TableCollection<T> { | |||
There was a problem hiding this comment.
Why is this overriding the signature of the base class?
There was a problem hiding this comment.
so right now BaseCollection creates a new BaseCollection when filtering:
react-spectrum/packages/@react-aria/collections/src/BaseCollection.ts
Lines 286 to 288 in 57e57e0
however, when Table filters itself, it need to actually preserve its columns/etc since it is actually only performing filtering on the body. Thus if I were to change the above line to
newCollection = new (this.constructor as typeof BaseCollection<T>)();
then we lose the column information since we don't run commit. If I were to instead change the above line to
newCollection = this.clone();
it fixes the issue with Table, but breaks Menu/ListBox filtering due to some wonkiness in filterChildren with my separator filtering, digging
EDIT: hm, thats not quite right, commit does get called, just before the filtering happens
There was a problem hiding this comment.
ah right, so if we don't create a new BaseCollection and just clone it, then walking from the new firstKey will hit nodes that don't exist anymore since they stick around. This is actually a problem with the current Table filter strategy, I need to somehow either rebuild the entire TableCollection from scratch on filter, or initialize a TableCollection that retains the columns and only filters the body
* autoimport.... * replace internal autocomplete context * add FieldInputContext in place of input context and search/textfield context in autocomplete * fix build * removing erroneous autoimports * add ability for user to provide independent filter text * fix lint * fix some more tests * bring back controlled input value at autocomplete level * adding prop to disable virtual focus * another stab at the types * clear autocomplete contexts so that they dont leak to nested collections * add tests for disallowVirtualFocus works with listbox and menu * fix types * refactor CollectionNode to read from static property and properly clone from subclass * naming from reviews and moving contexts out of autocomplete * review comments * properly add all descendants of a cloned node when filtering fixes case where a filtered table keyboard navigation was broken since we had cloned the old collection rather than creating a new one from scratch
|
Build successful! 🎉 |
…um into baseCollection_filter
|
Build successful! 🎉 |
There was a problem hiding this comment.
If I go to Autocomplete, dynamic menu and keyboard nav down to Appearance, then press Enter, I can't arrow up/down from there without typing a letter first.
|
@reidbarber oh good catch, I'll have to dig |
fixes case where opening a nested autocomplete subdialog in a autocomplete menu via ENTER didnt allow the user to navigate the subdialogs options via keyboard
|
Build successful! 🎉 |
| let { | ||
| inputRef, | ||
| collectionRef, | ||
| filter, | ||
| disableAutoFocusFirst = false | ||
| disableAutoFocusFirst = false, | ||
| disableVirtualFocus = false |
There was a problem hiding this comment.
There was a problem hiding this comment.
There's another issue here, what does virtual focus look like for grid collections? what if someone puts an editable field in it? Maybe it's better that it's not virtual by default?
There was a problem hiding this comment.
Given the note I made above w/ regards to grid collections + virtual focus, do we want to move this out of unstable yet? Probably ok?
There was a problem hiding this comment.
same question move out of unstable? Haven't implemented the focus key reset behavior, was gonna do that in #8728
| // TODO: this is pretty specific to menu, will need to check if it is generic enough | ||
| // Will need to handle varying levels I assume but will revisit after I get searchable menu working for base menu | ||
| // TODO: an alternative is to simply walk the collection and add all item nodes that match the filter and any sections/separators we encounter | ||
| // to an array, then walk that new array and fix all the next/Prev keys while adding them to the new collection | ||
| UNSTABLE_filter(filterFn: (nodeValue: string) => boolean): BaseCollection<T> { |
There was a problem hiding this comment.
I forgot the decision around the UNSTABLE_filter here, did we want to keep it around since it has different logic from the new version of filter? Kinda annoying if so...
| // This context is to pass the register and filter down to whatever collection component is wrapped by the Autocomplete | ||
| // TODO: export from RAC, but rename to something more appropriate | ||
| export const UNSTABLE_InternalAutocompleteContext = createContext<InternalAutocompleteContextValue | null>(null); |
There was a problem hiding this comment.
Another instance UNSTABLE we'll have to handle, the signature changed so can't just re-export
| /** An immutable object representing a Node in a Collection. */ | ||
| export class CollectionNode<T> implements Node<T> { | ||
| static readonly type; |
There was a problem hiding this comment.
| static readonly type; | |
| static readonly type: string; |
| @@ -99,6 +99,7 @@ interface SectionContextValue { | |||
|
|
|||
| export const SectionContext = createContext<SectionContextValue | null>(null); | |||
|
|
|||
| // TODO: should I update this since it is deprecated? | |||
|
Build successful! 🎉 |
## API Changes
react-aria-components/react-aria-components:Autocomplete-Autocomplete {
+Autocomplete <T extends {}> {
children: ReactNode
defaultInputValue?: string
disableAutoFocusFirst?: boolean = false
- filter?: (string, string) => boolean
+ disableVirtualFocus?: boolean = false
+ filter?: (string, string, Node<{}>) => boolean
inputValue?: string
onInputChange?: (string) => void
slot?: string | null
}/react-aria-components:UNSTABLE_createLeafComponent UNSTABLE_createLeafComponent <E extends Element, P extends {}> {
- type: string
+ CollectionNodeClass: {}<any> | string
render: (P, ForwardedRef<E>, any) => ReactElement | null
returnVal: undefined
}/react-aria-components:UNSTABLE_createBranchComponent UNSTABLE_createBranchComponent <E extends Element, P extends {
children?: any
}, T extends {}> {
- type: string
+ CollectionNodeClass: {}<any> | string
render: (P, ForwardedRef<E>, Node<T>) => ReactElement | null
useChildren: (P) => ReactNode
returnVal: undefined
}/react-aria-components:AutocompleteProps-AutocompleteProps {
+AutocompleteProps <T = {}> {
children: ReactNode
defaultInputValue?: string
disableAutoFocusFirst?: boolean = false
- filter?: (string, string) => boolean
+ disableVirtualFocus?: boolean = false
+ filter?: (string, string, Node<T>) => boolean
inputValue?: string
onInputChange?: (string) => void
slot?: string | null
}/react-aria-components:WaterfallLayoutOptions WaterfallLayoutOptions {
dropIndicatorThickness?: number = 2
maxColumns?: number = Infinity
- maxHorizontalSpace?: number = Infinity
maxItemSize?: Size = Infinity
minItemSize?: Size = 200 x 200
minSpace?: Size = 18 x 18
}@react-aria/autocomplete/@react-aria/autocomplete:useAutocomplete-useAutocomplete {
+useAutocomplete <T> {
- props: AriaAutocompleteOptions
+ props: AriaAutocompleteOptions<T>
state: AutocompleteState
returnVal: undefined
}/@react-aria/autocomplete:AriaAutocompleteProps-AriaAutocompleteProps {
+AriaAutocompleteProps <T> {
children: ReactNode
defaultInputValue?: string
disableAutoFocusFirst?: boolean = false
- filter?: (string, string) => boolean
+ disableVirtualFocus?: boolean = false
+ filter?: (string, string, Node<T>) => boolean
inputValue?: string
onInputChange?: (string) => void
}/@react-aria/autocomplete:AriaAutocompleteOptions-AriaAutocompleteOptions {
+AriaAutocompleteOptions <T> {
collectionRef: RefObject<HTMLElement | null>
defaultInputValue?: string
disableAutoFocusFirst?: boolean = false
- filter?: (string, string) => boolean
+ disableVirtualFocus?: boolean = false
+ filter?: (string, string, Node<T>) => boolean
inputRef: RefObject<HTMLInputElement | null>
inputValue?: string
onInputChange?: (string) => void
}/@react-aria/autocomplete:AutocompleteAria-AutocompleteAria {
+AutocompleteAria <T> {
collectionProps: CollectionOptions
collectionRef: RefObject<HTMLElement | null>
- filter?: (string) => boolean
- textFieldProps: AriaTextFieldProps
+ filter?: (string, Node<T>) => boolean
+ textFieldProps: AriaTextFieldProps<FocusableElement>
}@react-aria/collections/@react-aria/collections:createLeafComponent createLeafComponent <E extends Element, P extends {}> {
- type: string
+ CollectionNodeClass: {}<any> | string
render: (P, ForwardedRef<E>, any) => ReactElement | null
returnVal: undefined
}/@react-aria/collections:createBranchComponent createBranchComponent <E extends Element, P extends {
children?: any
}, T extends {}> {
- type: string
+ CollectionNodeClass: {}<any> | string
render: (P, ForwardedRef<E>, Node<T>) => ReactElement | null
useChildren: (P) => ReactNode
returnVal: undefined
}/@react-aria/collections:BaseCollection BaseCollection <T> {
- UNSTABLE_filter: ((string) => boolean) => BaseCollection<T>
+ addDescendants: (CollectionNode<T>, BaseCollection<T>) => void
addNode: (CollectionNode<T>) => void
at: () => Node<T>
clone: () => this
commit: (Key | null, Key | null, any) => void
+ filter: (FilterFn<T>) => this
getChildren: (Key) => Iterable<Node<T>>
getFirstKey: () => Key | null
getItem: (Key) => Node<T> | null
getKeyAfter: (Key) => Key | null
getKeys: () => IterableIterator<Key>
getLastKey: () => Key | null
removeNode: (Key) => void
size: number
undefined: () => IterableIterator<Node<T>>
}/@react-aria/collections:CollectionNode CollectionNode <T> {
aria-label?: string
childNodes: Iterable<Node<T>>
- clone: () => CollectionNode<T>
+ clone: () => this
colIndex: number | null
colSpan: number | null
- constructor: (string, Key) => void
+ constructor: (Key) => void
+ filter: (BaseCollection<T>, BaseCollection<T>, FilterFn<T>) => CollectionNode<T> | null
firstChildKey: Key | null
hasChildNodes: boolean
index: number
key: Key
level: number
nextKey: Key | null
parentKey: Key | null
prevKey: Key | null
props: any
render?: (Node<any>) => ReactElement
rendered: ReactNode
textValue: string
type: string
value: T | null
}/@react-aria/collections:ItemNode+ItemNode <T> {
+ aria-label?: string
+ childNodes: Iterable<Node<T>>
+ clone: () => this
+ colIndex: number | null
+ colSpan: number | null
+ constructor: (Key) => void
+ filter: (BaseCollection<T>, BaseCollection<T>, FilterFn<T>) => ItemNode<T> | null
+ firstChildKey: Key | null
+ hasChildNodes: boolean
+ index: number
+ key: Key
+ lastChildKey: Key | null
+ level: number
+ nextKey: Key | null
+ parentKey: Key | null
+ prevKey: Key | null
+ props: any
+ render?: (Node<any>) => ReactElement
+ rendered: ReactNode
+ textValue: string
+ type: any
+ value: T | null
+}/@react-aria/collections:SectionNode+SectionNode <T> {
+ aria-label?: string
+ childNodes: Iterable<Node<T>>
+ clone: () => this
+ colIndex: number | null
+ colSpan: number | null
+ constructor: (Key) => void
+ filter: (BaseCollection<T>, BaseCollection<T>, FilterFn<T>) => SectionNode<T> | null
+ firstChildKey: Key | null
+ hasChildNodes: boolean
+ index: number
+ key: Key
+ lastChildKey: Key | null
+ level: number
+ nextKey: Key | null
+ parentKey: Key | null
+ prevKey: Key | null
+ props: any
+ render?: (Node<any>) => ReactElement
+ rendered: ReactNode
+ textValue: string
+ type: any
+ value: T | null
+}/@react-aria/collections:FilterLessNode+FilterLessNode <T> {
+ aria-label?: string
+ childNodes: Iterable<Node<T>>
+ clone: () => this
+ colIndex: number | null
+ colSpan: number | null
+ constructor: (Key) => void
+ filter: (BaseCollection<T>, BaseCollection<T>, FilterFn<T>) => FilterLessNode<T> | null
+ firstChildKey: Key | null
+ hasChildNodes: boolean
+ index: number
+ key: Key
+ lastChildKey: Key | null
+ level: number
+ nextKey: Key | null
+ parentKey: Key | null
+ prevKey: Key | null
+ props: any
+ render?: (Node<any>) => ReactElement
+ rendered: ReactNode
+ textValue: string
+ type: string
+ value: T | null
+}/@react-aria/collections:LoaderNode+LoaderNode {
+ aria-label?: string
+ childNodes: Iterable<Node<T>>
+ clone: () => this
+ colIndex: number | null
+ colSpan: number | null
+ constructor: (Key) => void
+ filter: (BaseCollection<T>, BaseCollection<T>, FilterFn<T>) => FilterLessNode<T> | null
+ firstChildKey: Key | null
+ hasChildNodes: boolean
+ index: number
+ key: Key
+ lastChildKey: Key | null
+ level: number
+ nextKey: Key | null
+ parentKey: Key | null
+ prevKey: Key | null
+ props: any
+ render?: (Node<any>) => ReactElement
+ rendered: ReactNode
+ textValue: string
+ type: any
+ value: T | null
+}/@react-aria/collections:HeaderNode+HeaderNode {
+ aria-label?: string
+ childNodes: Iterable<Node<T>>
+ clone: () => this
+ colIndex: number | null
+ colSpan: number | null
+ constructor: (Key) => void
+ filter: (BaseCollection<T>, BaseCollection<T>, FilterFn<T>) => FilterLessNode<T> | null
+ firstChildKey: Key | null
+ hasChildNodes: boolean
+ index: number
+ key: Key
+ lastChildKey: Key | null
+ level: number
+ nextKey: Key | null
+ parentKey: Key | null
+ prevKey: Key | null
+ props: any
+ render?: (Node<any>) => ReactElement
+ rendered: ReactNode
+ textValue: string
+ type: any
+ value: T | null
+}@react-spectrum/s2/@react-spectrum/s2:Autocomplete-Autocomplete {
+Autocomplete <T extends {}> {
children: ReactNode
defaultInputValue?: string
disableAutoFocusFirst?: boolean = false
- filter?: (string, string) => boolean
+ disableVirtualFocus?: boolean = false
+ filter?: (string, string, Node<{}>) => boolean
inputValue?: string
onInputChange?: (string) => void
slot?: string | null
}/@react-spectrum/s2:AutocompleteProps-AutocompleteProps {
+AutocompleteProps <T = {}> {
children: ReactNode
defaultInputValue?: string
disableAutoFocusFirst?: boolean = false
- filter?: (string, string) => boolean
+ disableVirtualFocus?: boolean = false
+ filter?: (string, string, Node<T>) => boolean
inputValue?: string
onInputChange?: (string) => void
slot?: string | null
}@react-stately/layout/@react-stately/layout:WaterfallLayoutOptions WaterfallLayoutOptions {
dropIndicatorThickness?: number = 2
maxColumns?: number = Infinity
- maxHorizontalSpace?: number = Infinity
maxItemSize?: Size = Infinity
minItemSize?: Size = 200 x 200
minSpace?: Size = 18 x 18
}@react-stately/list/@react-stately/list:UNSTABLE_useFilteredListState UNSTABLE_useFilteredListState <T extends {}> {
state: ListState<T>
- filter: (string) => boolean | null | undefined
+ filterFn: (string, Node<T>) => boolean | null | undefined
returnVal: undefined
}@react-stately/table/@react-stately/table:UNSTABLE_useFilteredTableState+UNSTABLE_useFilteredTableState <T extends {}> {
+ state: TableState<T>
+ filterFn: (string, Node<T>) => boolean | null | undefined
+ returnVal: undefined
+} |
| @@ -280,6 +291,9 @@ export function useAutocomplete(props: AriaAutocompleteOptions, state: Autocompl | |||
| } | |||
| break; | |||
| } | |||
| } else { | |||
| // TODO: check if we can do this, want to stop textArea from using its default Enter behavior so items are properly triggered | |||
| e.preventDefault(); | |||
There was a problem hiding this comment.
only do it for Enter?
also, maybe allow Opt+Enter? or essentially only if no modifies are pressed?
Closes
✅ Pull Request Checklist:
📝 Test Instructions:
In the RAC storybook, test that filtering still works as expected for Autocomplete wrapped Menu/Listbox. Also test the Autocomplete GridList/Table/TagGroup/custom node filter stories work as expected (aka contents are filtered when the user types in the field). Note that virtual focus isn't supported for these grid collection components since Left/Right arrow is overloaded if so (would navigate the collection and move the text input cursor)
Some things to look out for is that loading spinners shouldn't be filtered out, keyboard navigation should still all work as expected (especially in nested menus), and that sections/dividers shouldn't stick around if they aren't needed (e.g. sections shouldn't remain if all of their contents are filtered out, and dividers shouldn't remain if they don't have content before/after them)
🧢 Your Project:
RSP