-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat: (React Aria) Implement filtering on a per CollectionNode basis #8641
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good, and thank you so much for going the extra mile!
Build successful! 🎉 |
…wrapped collection is filterable
…g for CollectionNodeClass
Build successful! 🎉 |
// TODO: naming, but essentially these nodes shouldn't be affected by filtering (BaseNode)? | ||
// Perhaps this filter logic should be in CollectionNode instead and the current logic of CollectionNode's filter should move to Table | ||
export class FilterLessNode<T> extends CollectionNode<T> { |
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.
open to opinions on the below nodes (naming and if BaseCollection's filter should instead be "do nothing")
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.
PersistentNode
, matches our other name for virtualised items which stick around when others do not
StaticNode
, maybe misleading?
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.
PersistentNode
does sound better, but I'm a bit worried that it will be confusing with virtualized behavior like you mentioned (it only sticks around when a filter operation happens, not really virtualizer specific).
export function UNSTABLE_useFilteredTableState<T extends object>(state: TableState<T>, filterFn: ((nodeValue: string, node: Node<T>) => boolean) | null | undefined): TableState<T> { | ||
let collection = useMemo(() => filterFn ? state.collection.filter!(filterFn) : state.collection, [state.collection, filterFn]) as ITableCollection<T>; | ||
let selectionManager = state.selectionManager.withCollection(collection); | ||
// TODO: handle focus key reset? That logic is in useGridState |
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.
Can copy over the focus key reset logic but you have to tab out of the table to filter it so maybe not too bad
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.
is that true? or can something else "control" the filtering and therefor trigger it as an async functionality while a user is inside the table?
or might it be that there's a network request that needs to complete before the filter and while waiting for that to happen, the user navigates into the table?
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.
fair point, I'll probably look to pull in some fashion of focused key handling in here when we handle the Autocomplete + grid navigation + virtual focus. The logic will probably be much simpler, perhaps might be best to reset to the first key if we can't find the original key since the contents of the newly filtered table may not even be related to the previous 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.
Found an interesting related situation, in the case where the item was moved in the autocomplete from somewhere in the middle of the collection to the "recently selected" section at the top, virtual focus got lost and we never scrolled up to the new location
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.
good catch, looks like this actually used to happen: https://reactspectrum.blob.core.windows.net/reactspectrum/2ee02a0afccfcbb4611e3f4085739ce80852d133/storybook/index.html?path=/story/react-aria-components-autocomplete--autocomplete-in-popover-dialog-trigger&providerSwitcher-express=false. Seems like the problem is actually just the scrolling since the input still has the proper active descendant and the moved item is still in the DOM it seems.
// TODO: maybe make a general loader node | ||
class GridListLoaderNode extends FilterLessNode<any> { |
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 now just using FilterLessNode, but can make a general LoaderNode if the logic diverges in the future
Build successful! 🎉 |
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.
first set of review comments, mostly confusion around these classes
// TODO: naming, but essentially these nodes shouldn't be affected by filtering (BaseNode)? | ||
// Perhaps this filter logic should be in CollectionNode instead and the current logic of CollectionNode's filter should move to Table | ||
export class FilterLessNode<T> extends CollectionNode<T> { |
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.
PersistentNode
, matches our other name for virtualised items which stick around when others do not
StaticNode
, maybe misleading?
import React, {createContext, ForwardedRef, HTMLAttributes} from 'react'; | ||
|
||
export const HeaderContext = createContext<ContextValue<HTMLAttributes<HTMLElement>, HTMLElement>>({}); | ||
|
||
export const Header = /*#__PURE__*/ createLeafComponent('header', function Header(props: HTMLAttributes<HTMLElement>, ref: ForwardedRef<HTMLElement>) { | ||
class HeaderNode extends FilterLessNode<unknown> { |
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 opinionated?
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.
yep, in the same vein as https://github.com/adobe/react-spectrum/pull/8641/files#r2252805272, will most likely let the user customize the desired filter behavior here in the future
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.
ok, so long as the intention is more of a default i guess
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is SeparatorNode exported, but HeaderNode isn't? I assume adding this here was an accident?
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.
needed in S2 Combobox's Divider implementation
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.
hmmm torn on exporting it vs recreating it since we recreated the Separator anyways, we're not using the RAC 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.
I'll go ahead and recreate it
@@ -99,6 +99,7 @@ interface SectionContextValue { | |||
|
|||
export const SectionContext = createContext<SectionContextValue | null>(null); | |||
|
|||
// TODO: should I update this since it is deprecated? |
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 think I'm ok not updating it
readonly type: string | ||
}; | ||
|
||
function createCollectionNodeClass(type: string): CollectionNodeClass<any> { |
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.
function createCollectionNodeClass(type: string): CollectionNodeClass<any> { | |
function createCollectionNodeClass(type: string): CollectionNode<any> { |
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 not producing a CollectionNode since you can't set the type after calling createCollectionNodeClass
let NodeClass = function (key: Key) { | ||
return new CollectionNode(type, key); | ||
} as any; | ||
NodeClass.type = type; | ||
return NodeClass; |
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 address the above, maybe we can remove the need for this
or simplify it, i'm not sure why it's wrapped in a function
filter(collection: BaseCollection<any>, newCollection: BaseCollection<any>): CollectionNode<any> | null { | ||
if (newCollection.getItem(this.prevKey!)) { | ||
return this.clone(); | ||
} |
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 you have parentKey or nextKey, could you decide to return null here? saw you had a question of if you could move that logic in here
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 problem is that at this point where you are calling filter
on the separator, we haven't performed filter
for the nodes after it so we don't know if the nextKey
will still be in the filtered 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.
ahh... no look ahead, got it
could add a callback to check it after the collection is complete? almost like a second pass, but not over the entire collection, so shouldn't be a performance hit
I was also thinking, might be able to do this with css, just leave all the separators in the collection, assign them some data-attribute that matches a section/items and check in the css if it has any siblings matching that data attribute, otherwise hide it
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 guess I could do the callback, trying to think about how filterChildren would go about calling that callback before it returns the first/last node (maybe filter would return a callback alongside the CollectionNode)? Still feels easier to handle this logic in filterChildren though...
As for the css approach, that would mean the user would need to implement that no? Wouldn't be too hard I guess, but kinda annoying that they would need to know to do so.
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, not ideal
It would be nice if we could queue additional checks, will think on it some more
Build successful! 🎉 |
d3f22fb
to
d2b5e51
Compare
Build successful! 🎉 |
Build successful! 🎉 |
Build successful! 🎉 |
Build successful! 🎉 |
Build successful! 🎉 |
}; | ||
|
||
return ( | ||
<Autocomplete filter={filter}> |
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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! 🎉 |
## API Changes
react-aria-components/react-aria-components:Autocomplete-Autocomplete {
+Autocomplete <T extends {}> {
children: ReactNode
defaultInputValue?: string
disableAutoFocusFirst?: boolean = false
- filter?: (string, string) => boolean
+ 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:Popover Popover {
aria-describedby?: string
aria-details?: string
aria-label?: string
aria-labelledby?: string
arrowBoundaryOffset?: number = 0
- arrowRef?: RefObject<Element | null>
boundaryElement?: Element = document.body
children?: ChildrenOrFunction<PopoverRenderProps>
className?: ClassNameOrFunction<PopoverRenderProps>
containerPadding?: number = 12
defaultOpen?: boolean
isEntering?: boolean
isExiting?: boolean
isKeyboardDismissDisabled?: boolean = false
isNonModal?: boolean
isOpen?: boolean
maxHeight?: number
offset?: number = 8
onOpenChange?: (boolean) => void
placement?: Placement = 'bottom'
scrollRef?: RefObject<Element | null> = overlayRef
shouldCloseOnInteractOutside?: (Element) => boolean
shouldFlip?: boolean = true
shouldUpdatePosition?: boolean = true
slot?: string | null
style?: StyleOrFunction<PopoverRenderProps>
trigger?: string
triggerRef?: RefObject<Element | null>
} /react-aria-components:VisuallyHidden-VisuallyHidden {
- children?: ReactNode
- className?: string | undefined
- elementType?: string | JSXElementConstructor<any> = 'div'
- id?: string | undefined
- isFocusable?: boolean
- role?: AriaRole | undefined
- style?: CSSProperties | undefined
- tabIndex?: number | undefined
-} /react-aria-components:AutocompleteProps-AutocompleteProps {
+AutocompleteProps <T> {
children: ReactNode
defaultInputValue?: string
disableAutoFocusFirst?: boolean = false
- filter?: (string, string) => boolean
+ filter?: (string, string, Node<T>) => boolean
inputValue?: string
onInputChange?: (string) => void
slot?: string | null
} /react-aria-components:PopoverProps PopoverProps {
aria-describedby?: string
aria-details?: string
aria-label?: string
aria-labelledby?: string
arrowBoundaryOffset?: number = 0
- arrowRef?: RefObject<Element | null>
boundaryElement?: Element = document.body
children?: ChildrenOrFunction<PopoverRenderProps>
className?: ClassNameOrFunction<PopoverRenderProps>
containerPadding?: number = 12
defaultOpen?: boolean
isEntering?: boolean
isExiting?: boolean
isKeyboardDismissDisabled?: boolean = false
isNonModal?: boolean
isOpen?: boolean
maxHeight?: number
offset?: number = 8
onOpenChange?: (boolean) => void
placement?: Placement = 'bottom'
scrollRef?: RefObject<Element | null> = overlayRef
shouldCloseOnInteractOutside?: (Element) => boolean
shouldFlip?: boolean = true
shouldUpdatePosition?: boolean = true
slot?: string | null
style?: StyleOrFunction<PopoverRenderProps>
trigger?: string
triggerRef?: RefObject<Element | null>
} /react-aria-components:GridLayoutOptions GridLayoutOptions {
dropIndicatorThickness?: number = 2
maxColumns?: number = Infinity
- maxHorizontalSpace?: number = Infinity
maxItemSize?: Size = Infinity
minItemSize?: Size = 200 x 200
minSpace?: Size = 18 x 18
preserveAspectRatio?: boolean = false /react-aria-components:SeparatorNode+SeparatorNode {
+ aria-label?: string
+ childNodes: Iterable<Node<T>>
+ clone: () => CollectionNode<T>
+ colIndex: number | null
+ colSpan: number | null
+ constructor: (Key) => void
+ filter: (BaseCollection<any>, BaseCollection<any>) => CollectionNode<any> | 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
+} @internationalized/date/@internationalized/date:setLocalTimeZone-setLocalTimeZone {
- timeZone: string
- returnVal: undefined
-} /@internationalized/date:resetLocalTimeZone-resetLocalTimeZone {
- returnVal: undefined
-} @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
+ 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
+ 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
+ filter?: (string, Node<T>) => boolean
textFieldProps: AriaTextFieldProps
} @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>
addNode: (CollectionNode<T>) => void
at: () => Node<T>
clone: () => this
commit: (Key | null, Key | null, any) => void
+ filter: (FilterFn<T>, BaseCollection<T>) => BaseCollection<T>
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>
colIndex: number | null
colSpan: number | null
constructor: (string, 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: () => CollectionNode<T>
+ 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: () => CollectionNode<T>
+ 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: () => CollectionNode<T>
+ colIndex: number | null
+ colSpan: number | null
+ constructor: (string, 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/overlays/@react-aria/overlays:AriaPositionProps AriaPositionProps {
arrowBoundaryOffset?: number = 0
- arrowRef?: RefObject<Element | null>
arrowSize?: number = 0
boundaryElement?: Element = document.body
containerPadding?: number = 12
crossOffset?: number = 0
maxHeight?: number
offset?: number = 0
onClose?: () => void | null
overlayRef: RefObject<Element | null>
placement?: Placement = 'bottom'
scrollRef?: RefObject<Element | null> = overlayRef
shouldFlip?: boolean = true
shouldUpdatePosition?: boolean = true
targetRef: RefObject<Element | null>
} /@react-aria/overlays:PositionAria PositionAria {
arrowProps: DOMAttributes
overlayProps: DOMAttributes
placement: PlacementAxis | null
- triggerOrigin: {
- x: number
- y: number
-} | null
updatePosition: () => void
} /@react-aria/overlays:AriaPopoverProps AriaPopoverProps {
arrowBoundaryOffset?: number = 0
- arrowRef?: RefObject<Element | null>
arrowSize?: number = 0
boundaryElement?: Element = document.body
containerPadding?: number = 12
crossOffset?: number = 0
isKeyboardDismissDisabled?: boolean = false
isNonModal?: boolean
maxHeight?: number
offset?: number = 0
placement?: Placement = 'bottom'
popoverRef: RefObject<Element | null>
scrollRef?: RefObject<Element | null> = overlayRef
shouldCloseOnInteractOutside?: (Element) => boolean
shouldFlip?: boolean = true
shouldUpdatePosition?: boolean = true
triggerRef: RefObject<Element | null>
} /@react-aria/overlays:PopoverAria PopoverAria {
arrowProps: DOMAttributes
placement: PlacementAxis | null
popoverProps: DOMAttributes
- triggerOrigin: {
- x: number
- y: number
-} | null
underlayProps: DOMAttributes
} @react-spectrum/overlays/@react-spectrum/overlays:Popover Popover {
UNSAFE_className?: string
UNSAFE_style?: CSSProperties
alignSelf?: Responsive<'auto' | 'normal' | 'start' | 'end' | 'center' | 'flex-start' | 'flex-end' | 'self-start' | 'self-end' | 'stretch'>
arrowBoundaryOffset?: number = 0
- arrowRef?: RefObject<Element | null>
arrowSize?: number = 0
bottom?: Responsive<DimensionValue>
boundaryElement?: Element = document.body
children: ReactNode
containerPadding?: number = 12
crossOffset?: number = 0
disableFocusManagement?: boolean
enableBothDismissButtons?: boolean
end?: Responsive<DimensionValue>
flex?: Responsive<string | number | boolean>
flexBasis?: Responsive<number | string>
flexGrow?: Responsive<number>
flexShrink?: Responsive<number>
gridArea?: Responsive<string>
gridColumn?: Responsive<string>
gridColumnEnd?: Responsive<string>
gridColumnStart?: Responsive<string>
gridRow?: Responsive<string>
gridRowEnd?: Responsive<string>
gridRowStart?: Responsive<string>
groupRef?: RefObject<Element | null>
height?: Responsive<DimensionValue>
hideArrow?: boolean
isDisabled?: boolean
isHidden?: Responsive<boolean>
isKeyboardDismissDisabled?: boolean = false
isNonModal?: boolean
justifySelf?: Responsive<'auto' | 'normal' | 'start' | 'end' | 'flex-start' | 'flex-end' | 'self-start' | 'self-end' | 'center' | 'left' | 'right' | 'stretch'>
left?: Responsive<DimensionValue>
margin?: Responsive<DimensionValue>
marginBottom?: Responsive<DimensionValue>
marginEnd?: Responsive<DimensionValue>
marginStart?: Responsive<DimensionValue>
marginTop?: Responsive<DimensionValue>
marginX?: Responsive<DimensionValue>
marginY?: Responsive<DimensionValue>
maxHeight?: Responsive<DimensionValue>
maxWidth?: Responsive<DimensionValue>
minHeight?: Responsive<DimensionValue>
minWidth?: Responsive<DimensionValue>
offset?: number = 0
onBlurWithin?: (FocusEvent) => void
onDismissButtonPress?: () => void
onEnter?: () => void
onEntered?: () => void
onEntering?: () => void
onExit?: () => void
onExited?: () => void
onExiting?: () => void
onFocusWithin?: (FocusEvent) => void
onFocusWithinChange?: (boolean) => void
order?: Responsive<number>
placement?: Placement = 'bottom'
position?: Responsive<'static' | 'relative' | 'absolute' | 'fixed' | 'sticky'>
right?: Responsive<DimensionValue>
scrollRef?: RefObject<Element | null> = overlayRef
shouldCloseOnInteractOutside?: (Element) => boolean
shouldContainFocus?: boolean
shouldFlip?: boolean = true
shouldUpdatePosition?: boolean = true
start?: Responsive<DimensionValue>
state: OverlayTriggerState
top?: Responsive<DimensionValue>
triggerRef: RefObject<Element | null>
width?: Responsive<DimensionValue>
zIndex?: Responsive<number>
} @react-spectrum/s2/@react-spectrum/s2:Autocomplete-Autocomplete {
+Autocomplete <T extends {}> {
children: ReactNode
defaultInputValue?: string
disableAutoFocusFirst?: boolean = false
- filter?: (string, string) => boolean
+ filter?: (string, string, Node<{}>) => boolean
inputValue?: string
onInputChange?: (string) => void
slot?: string | null
} /@react-spectrum/s2:PopoverProps PopoverProps {
UNSAFE_className?: UnsafeClassName
UNSAFE_style?: CSSProperties
aria-describedby?: string
aria-details?: string
aria-label?: string
aria-labelledby?: string
- arrowRef?: RefObject<Element | null>
boundaryElement?: Element = document.body
children?: ChildrenOrFunction<PopoverRenderProps>
className?: ClassNameOrFunction<PopoverRenderProps>
containerPadding?: number = 12
defaultOpen?: boolean
hideArrow?: boolean = false
isEntering?: boolean
isExiting?: boolean
isOpen?: boolean
maxHeight?: number
offset?: number = 8
onOpenChange?: (boolean) => void
placement?: Placement = 'bottom'
scrollRef?: RefObject<Element | null> = overlayRef
shouldFlip?: boolean = true
size?: 'S' | 'M' | 'L'
slot?: string | null
style?: StyleOrFunction<PopoverRenderProps>
styles?: StyleString
trigger?: string
triggerRef?: RefObject<Element | null>
} /@react-spectrum/s2:AutocompleteProps-AutocompleteProps {
+AutocompleteProps <T> {
children: ReactNode
defaultInputValue?: string
disableAutoFocusFirst?: boolean = false
- filter?: (string, string) => boolean
+ filter?: (string, string, Node<T>) => boolean
inputValue?: string
onInputChange?: (string) => void
slot?: string | null
} @react-stately/data/@react-stately/data:ListData ListData <T> {
- addKeysToSelection: (Selection) => void
append: (Array<T>) => void
filterText: string
getItem: (Key) => T | undefined
insert: (number, Array<T>) => void
insertAfter: (Key, Array<T>) => void
insertBefore: (Key, Array<T>) => void
items: Array<T>
move: (Key, number) => void
moveAfter: (Key, Iterable<Key>) => void
moveBefore: (Key, Iterable<Key>) => void
prepend: (Array<T>) => void
remove: (Array<Key>) => void
- removeKeysFromSelection: (Selection) => void
removeSelectedItems: () => void
selectedKeys: Selection
setFilterText: (string) => void
setSelectedKeys: (Selection) => void
} /@react-stately/data:AsyncListData AsyncListData <T> {
- addKeysToSelection: (Selection) => void
append: (Array<T>) => void
error?: Error
filterText: string
getItem: (Key) => T | undefined
insert: (number, Array<T>) => void
insertAfter: (Key, Array<T>) => void
insertBefore: (Key, Array<T>) => void
isLoading: boolean
items: Array<T>
loadMore: () => void
loadingState: LoadingState
move: (Key, number) => void
moveAfter: (Key, Iterable<Key>) => void
moveBefore: (Key, Iterable<Key>) => void
prepend: (Array<T>) => void
reload: () => void
remove: (Array<Key>) => void
- removeKeysFromSelection: (Selection) => void
removeSelectedItems: () => void
selectedKeys: Selection
setFilterText: (string) => void
setSelectedKeys: (Selection) => void
sortDescriptor?: SortDescriptor
update: (Key, T) => void
} @react-stately/layout/@react-stately/layout:GridLayoutOptions GridLayoutOptions {
dropIndicatorThickness?: number = 2
maxColumns?: number = Infinity
- maxHorizontalSpace?: number = Infinity
maxItemSize?: Size = Infinity
minItemSize?: Size = 200 x 200
minSpace?: Size = 18 x 18
preserveAspectRatio?: boolean = false @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
+} |
let NodeClass = function (key: Key) { | ||
return new CollectionNode(type, key); | ||
} as any; | ||
NodeClass.type = type; | ||
return NodeClass; |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could memo the creation of the NodeClass as well if you wanted, then all the instances would be match
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.
updated in #8695
static readonly type = 'item'; | ||
|
||
constructor(key: Key) { | ||
super(ItemNode.type, key); |
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 the base CollectionNode class handle this? It could read this.constructor.type
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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> {} |
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.
subclass needed?
@@ -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 |
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.
👍
@@ -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; |
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.
?
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this overriding the signature of the base class?
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.
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
if (newCollection == null) { | |
newCollection = new BaseCollection<T>(); | |
} |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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