-
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?
Changes from 31 commits
c9ee11d
ac323e9
290514a
2969511
d8a6f06
639acd0
6eb1753
d1efa8d
77936ca
7991977
e615835
d02197e
361286b
432a43c
3ec3fd6
4a69d50
73a1971
1ead59b
90c2056
3a8301e
45a39c1
9d65d5b
19b695e
6066c6c
d2b5e51
2c89783
739e93f
3c2e92c
35b627e
9408aa9
8e75339
57e57e0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -17,6 +17,8 @@ export type Mutable<T> = { | |||||||||||||||||
-readonly[P in keyof T]: T[P] | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
type FilterFn<T> = (textValue: string, node: Node<T>) => boolean; | ||||||||||||||||||
|
||||||||||||||||||
/** An immutable object representing a Node in a Collection. */ | ||||||||||||||||||
export class CollectionNode<T> implements Node<T> { | ||||||||||||||||||
readonly type: string; | ||||||||||||||||||
|
@@ -65,8 +67,64 @@ export class CollectionNode<T> implements Node<T> { | |||||||||||||||||
node.render = this.render; | ||||||||||||||||||
node.colSpan = this.colSpan; | ||||||||||||||||||
node.colIndex = this.colIndex; | ||||||||||||||||||
node.filter = this.filter; | ||||||||||||||||||
return node; | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
filter(collection: BaseCollection<T>, newCollection: BaseCollection<T>, filterFn: FilterFn<T>): CollectionNode<T> | null { | ||||||||||||||||||
let [firstKey, lastKey] = filterChildren(collection, newCollection, this.firstChildKey, filterFn); | ||||||||||||||||||
let newNode: Mutable<CollectionNode<T>> = this.clone(); | ||||||||||||||||||
newNode.firstChildKey = firstKey; | ||||||||||||||||||
newNode.lastChildKey = lastKey; | ||||||||||||||||||
return newNode; | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
// 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> { | ||||||||||||||||||
Comment on lines
+83
to
+85
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||||||||||
// eslint-disable-next-line @typescript-eslint/no-unused-vars | ||||||||||||||||||
filter(collection: BaseCollection<T>, newCollection: BaseCollection<T>, filterFn: FilterFn<T>): FilterLessNode<T> | null { | ||||||||||||||||||
return this.clone(); | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
export class ItemNode<T> extends CollectionNode<T> { | ||||||||||||||||||
static readonly type = 'item'; | ||||||||||||||||||
|
||||||||||||||||||
constructor(key: Key) { | ||||||||||||||||||
super(ItemNode.type, key); | ||||||||||||||||||
} | ||||||||||||||||||
Comment on lines
+93
to
+97
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why static? I don't think we need this as a class field?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I originally just had them like the above, but typescript was giving me all kinds of hell in CollectionBuilder as a result due to the overrides There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmmm... because it only knew 'type = item'? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, so changing ItemNode in such a way then conflicts with the CollectionNodeClass definition since then it doesn't have a static There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Must've missed that usage, apologies. I thought it was only being used here. I see the other now so this makes more sense |
||||||||||||||||||
|
||||||||||||||||||
filter(collection: BaseCollection<T>, newCollection: BaseCollection<T>, filterFn: FilterFn<T>): ItemNode<T> | null { | ||||||||||||||||||
if (filterFn(this.textValue, this)) { | ||||||||||||||||||
return this.clone(); | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
return null; | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
export class SectionNode<T> extends CollectionNode<T> { | ||||||||||||||||||
static readonly type = 'section'; | ||||||||||||||||||
|
||||||||||||||||||
constructor(key: Key) { | ||||||||||||||||||
super(SectionNode.type, key); | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
filter(collection: BaseCollection<T>, newCollection: BaseCollection<T>, filterFn: FilterFn<T>): SectionNode<T> | null { | ||||||||||||||||||
let filteredSection = super.filter(collection, newCollection, filterFn); | ||||||||||||||||||
if (filteredSection) { | ||||||||||||||||||
if (filteredSection.lastChildKey !== null) { | ||||||||||||||||||
let lastChild = collection.getItem(filteredSection.lastChildKey); | ||||||||||||||||||
if (lastChild && lastChild.type !== 'header') { | ||||||||||||||||||
return filteredSection; | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
return null; | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
/** | ||||||||||||||||||
|
@@ -224,134 +282,65 @@ export class BaseCollection<T> implements ICollection<Node<T>> { | |||||||||||||||||
this.frozen = !isSSR; | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
// 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> { | ||||||||||||||||||
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; | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
filter(filterFn: FilterFn<T>, newCollection?: BaseCollection<T>): BaseCollection<T> { | ||||||||||||||||||
if (newCollection == null) { | ||||||||||||||||||
newCollection = new BaseCollection<T>(); | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
// Add newly filtered section to collection if it has any valid child nodes, otherwise remove it and its header if any | ||||||||||||||||||
if (lastChildInSection) { | ||||||||||||||||||
if (lastChildInSection.type !== 'header') { | ||||||||||||||||||
clonedSection.lastChildKey = lastChildInSection.key; | ||||||||||||||||||
|
||||||||||||||||||
// If the old prev section was filtered out, will need to attach to whatever came before | ||||||||||||||||||
// eslint-disable-next-line max-depth | ||||||||||||||||||
if (lastNode == null) { | ||||||||||||||||||
clonedSection.prevKey = null; | ||||||||||||||||||
} else if (lastNode.type === 'section' || lastNode.type === 'separator') { | ||||||||||||||||||
lastNode.nextKey = clonedSection.key; | ||||||||||||||||||
clonedSection.prevKey = lastNode.key; | ||||||||||||||||||
} | ||||||||||||||||||
clonedSection.nextKey = null; | ||||||||||||||||||
lastNode = clonedSection; | ||||||||||||||||||
newCollection.addNode(clonedSection); | ||||||||||||||||||
} else { | ||||||||||||||||||
if (newCollection.firstKey === clonedSection.key) { | ||||||||||||||||||
newCollection.firstKey = null; | ||||||||||||||||||
} | ||||||||||||||||||
newCollection.removeNode(lastChildInSection.key); | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
} else if (node.type === 'separator') { | ||||||||||||||||||
// will need to check if previous section key exists, if it does then we add the separator to the collection. | ||||||||||||||||||
// After the full collection is created we'll need to remove it it is the last node in the section (aka no following section after the separator) | ||||||||||||||||||
let clonedSeparator: Mutable<CollectionNode<T>> = (node as CollectionNode<T>).clone(); | ||||||||||||||||||
clonedSeparator.nextKey = null; | ||||||||||||||||||
if (lastNode?.type === 'section') { | ||||||||||||||||||
lastNode.nextKey = clonedSeparator.key; | ||||||||||||||||||
clonedSeparator.prevKey = lastNode.key; | ||||||||||||||||||
lastNode = clonedSeparator; | ||||||||||||||||||
newCollection.addNode(clonedSeparator); | ||||||||||||||||||
} | ||||||||||||||||||
} else { | ||||||||||||||||||
// At this point, the node is either a subdialogtrigger node or a standard row/item | ||||||||||||||||||
let clonedNode: Mutable<CollectionNode<T>> = (node as CollectionNode<T>).clone(); | ||||||||||||||||||
if (shouldKeepNode(clonedNode, filterFn, this, newCollection)) { | ||||||||||||||||||
if (newCollection.firstKey == null) { | ||||||||||||||||||
newCollection.firstKey = clonedNode.key; | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
if (lastNode != null && (lastNode.type !== 'section' && lastNode.type !== 'separator') && lastNode.parentKey === clonedNode.parentKey) { | ||||||||||||||||||
lastNode.nextKey = clonedNode.key; | ||||||||||||||||||
clonedNode.prevKey = lastNode.key; | ||||||||||||||||||
} else { | ||||||||||||||||||
clonedNode.prevKey = null; | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
clonedNode.nextKey = null; | ||||||||||||||||||
newCollection.addNode(clonedNode); | ||||||||||||||||||
lastNode = clonedNode; | ||||||||||||||||||
} | ||||||||||||||||||
let [firstKey, lastKey] = filterChildren(this, newCollection, this.firstKey, filterFn); | ||||||||||||||||||
newCollection.firstKey = firstKey; | ||||||||||||||||||
newCollection.lastKey = lastKey; | ||||||||||||||||||
return newCollection; | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
function filterChildren<T>(collection: BaseCollection<T>, newCollection: BaseCollection<T>, firstChildKey: Key | null, filterFn: FilterFn<T>): [Key | null, Key | null] { | ||||||||||||||||||
// loop over the siblings for firstChildKey | ||||||||||||||||||
// create new nodes based on calling node.filter for each child | ||||||||||||||||||
// if it returns null then don't include it, otherwise update its prev/next keys | ||||||||||||||||||
// add them to the newCollection | ||||||||||||||||||
if (firstChildKey == null) { | ||||||||||||||||||
return [null, null]; | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
let firstNode: Node<T> | null = null; | ||||||||||||||||||
let lastNode: Node<T> | null = null; | ||||||||||||||||||
let currentNode = collection.getItem(firstChildKey); | ||||||||||||||||||
|
||||||||||||||||||
while (currentNode != null) { | ||||||||||||||||||
let newNode: Mutable<CollectionNode<T>> | null = (currentNode as CollectionNode<T>).filter(collection, newCollection, filterFn); | ||||||||||||||||||
if (newNode != null) { | ||||||||||||||||||
newNode.nextKey = null; | ||||||||||||||||||
if (lastNode) { | ||||||||||||||||||
newNode.prevKey = lastNode.key; | ||||||||||||||||||
lastNode.nextKey = newNode.key; | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
if (lastNode?.type === 'separator' && lastNode.nextKey === null) { | ||||||||||||||||||
let lastSection; | ||||||||||||||||||
if (lastNode.prevKey != null) { | ||||||||||||||||||
lastSection = newCollection.getItem(lastNode.prevKey) as Mutable<CollectionNode<T>>; | ||||||||||||||||||
lastSection.nextKey = null; | ||||||||||||||||||
if (firstNode == null) { | ||||||||||||||||||
firstNode = newNode; | ||||||||||||||||||
} | ||||||||||||||||||
newCollection.removeNode(lastNode.key); | ||||||||||||||||||
lastNode = lastSection; | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
newCollection.lastKey = lastNode?.key || null; | ||||||||||||||||||
newCollection.addNode(newNode); | ||||||||||||||||||
lastNode = newNode; | ||||||||||||||||||
snowystinger marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
return newCollection; | ||||||||||||||||||
currentNode = currentNode.nextKey ? collection.getItem(currentNode.nextKey) : null; | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
function shouldKeepNode<T>(node: Node<T>, filterFn: (nodeValue: string) => boolean, oldCollection: BaseCollection<T>, newCollection: BaseCollection<T>): boolean { | ||||||||||||||||||
if (node.type === 'subdialogtrigger' || node.type === 'submenutrigger') { | ||||||||||||||||||
// Subdialog wrapper should only have one child, if it passes the filter add it to the new collection since we don't need to | ||||||||||||||||||
// do any extra handling for its first/next key | ||||||||||||||||||
let triggerChild = [...oldCollection.getChildren(node.key)][0]; | ||||||||||||||||||
if (triggerChild && filterFn(triggerChild.textValue)) { | ||||||||||||||||||
let clonedChild: Mutable<CollectionNode<T>> = (triggerChild as CollectionNode<T>).clone(); | ||||||||||||||||||
newCollection.addNode(clonedChild); | ||||||||||||||||||
return true; | ||||||||||||||||||
// TODO: this is pretty specific to dividers but doesn't feel like there is a good way to get around it since we only can know | ||||||||||||||||||
// to filter the last separator in a collection only after performing a filter for the rest of the contents after it | ||||||||||||||||||
// Its gross that it needs to live here, might be nice if somehow we could have this live in the separator code | ||||||||||||||||||
if (lastNode && lastNode.type === 'separator') { | ||||||||||||||||||
LFDanLu marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||
let prevKey = lastNode.prevKey; | ||||||||||||||||||
newCollection.removeNode(lastNode.key); | ||||||||||||||||||
|
||||||||||||||||||
if (prevKey) { | ||||||||||||||||||
lastNode = newCollection.getItem(prevKey) as Mutable<CollectionNode<T>>; | ||||||||||||||||||
lastNode.nextKey = null; | ||||||||||||||||||
} else { | ||||||||||||||||||
return false; | ||||||||||||||||||
lastNode = null; | ||||||||||||||||||
} | ||||||||||||||||||
} else if (node.type === 'header') { | ||||||||||||||||||
return true; | ||||||||||||||||||
} else { | ||||||||||||||||||
return filterFn(node.textValue); | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
return [firstNode?.key ?? null, lastNode?.key ?? 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.
For discussion, might be enough to just provide a subset of node information as mentioned above
Uh oh!
There was an error while loading. Please reload this page.
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, would also ask to hold off with this until the RFC. While it is definitely enough to sync collections, I do fancy the idea of being able to attach a node as context on another node - doing it all in one iteration would be great.
PS: I guess key in the end is always enough though since one can just retrieve the 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.
happy to hold off on paring it down for now, but on the flip side it is easier to go from exposing object containing a subset of the Node's values back to a Node if the need arises.