fix: S2 treeview haschildnodes #7694
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
## API Changes
react-aria-components/react-aria-components:UNSTABLE_TreeItem UNSTABLE_TreeItem <T extends {}> {
aria-label?: string
children: ReactNode
className?: string | ((TreeItemRenderProps & {
defaultClassName: string | undefined
})) => string
download?: boolean | string
+ hasChildItems?: boolean
href?: Href
hrefLang?: string
id?: Key
onHoverChange?: (boolean) => void
onHoverStart?: (HoverEvent) => void
ping?: string
referrerPolicy?: HTMLAttributeReferrerPolicy
rel?: string
routerOptions?: RouterOptions
style?: CSSProperties | ((TreeItemRenderProps & {
defaultStyle: CSSProperties
})) => CSSProperties | undefined
target?: HTMLAttributeAnchorTarget
textValue: string
value?: {}
}/react-aria-components:TreeItemProps TreeItemProps <T = {}> {
aria-label?: string
children: ReactNode
className?: string | ((TreeItemRenderProps & {
defaultClassName: string | undefined
})) => string
download?: boolean | string
+ hasChildItems?: boolean
href?: Href
hrefLang?: string
id?: Key
onHoverChange?: (boolean) => void
onHoverStart?: (HoverEvent) => void
ping?: string
referrerPolicy?: HTMLAttributeReferrerPolicy
rel?: string
routerOptions?: RouterOptions
style?: CSSProperties | ((TreeItemRenderProps & {
defaultStyle: CSSProperties
})) => CSSProperties | undefined
target?: HTMLAttributeAnchorTarget
textValue: string
value?: T
}@react-aria/gridlist/@react-aria/gridlist:AriaGridListItemOptions AriaGridListItemOptions {
+ hasChildItems?: boolean
isVirtualized?: boolean
node: Node<unknown>
shouldSelectOnPressUp?: boolean
}@react-aria/tree/@react-aria/tree:AriaTreeGridListItemOptions AriaTreeGridListItemOptions {
+ hasChildItems?: boolean
node: Node<unknown>
shouldSelectOnPressUp?: boolean
}@react-spectrum/s2/@react-spectrum/s2:TreeItemContent+TreeItemContent {
+
+}@react-spectrum/tree/@react-spectrum/tree:TreeItemContent+TreeItemContent {
+
+}/@react-spectrum/tree:Collection+Collection <T extends {}> {
+ addIdAndValue?: boolean
+ children?: ReactNode | ({}) => ReactNode
+ dependencies?: Array<any>
+ idScope?: Key
+ items?: Iterable<{}>
+} |
snowystinger
commented
Feb 4, 2025
| @@ -257,7 +257,18 @@ export class ElementNode<T> extends BaseNode<T> { | |||
| node.parentKey = this.parentNode instanceof ElementNode ? this.parentNode.node.key : null; | |||
| node.prevKey = this.previousSibling?.node.key ?? null; | |||
| node.nextKey = this.nextSibling?.node.key ?? null; | |||
| node.hasChildNodes = !!this.firstChild; | |||
|
|
|||
| // Check if this node has any child nodes, but specifically any that are items. | |||
There was a problem hiding this comment.
is this really a valid assumption? what about branch children? other types of items?
snowystinger
commented
Feb 4, 2025
| @@ -398,7 +398,7 @@ export class SelectionManager implements MultipleSelectionManager { | |||
| } | |||
|
|
|||
| // Add child keys. If cell selection is allowed, then include item children too. | |||
| if (item?.hasChildNodes && (this.allowsCellSelection || item.type !== 'item')) { | |||
| if (item && (item?.firstChildKey === undefined ? item?.hasChildNodes : item?.firstChildKey != null) && (this.allowsCellSelection || item.type !== 'item')) { | |||
There was a problem hiding this comment.
have to account for old collections which don't have firstChildKey on their nodes
snowystinger
commented
Feb 4, 2025
| @@ -215,6 +215,10 @@ export interface Node<T> { | |||
| prevKey?: Key | null, | |||
| /** The key of the node after this node. */ | |||
| nextKey?: Key | null, | |||
| /** The key of the first child node. */ | |||
| firstChildKey?: Key | null, | |||
There was a problem hiding this comment.
ok to expose these? doesn't match old collections
snowystinger
commented
Feb 4, 2025
| @@ -58,7 +58,7 @@ class TableCollection<T> extends BaseCollection<T> implements ITableCollection<T | |||
| switch (node.type) { | |||
| case 'column': | |||
| columnKeyMap.set(node.key, node); | |||
| if (!node.hasChildNodes) { | |||
| if (node.firstChildKey == null) { | |||
There was a problem hiding this comment.
this is technically what the old check was, is this still actually what we want to check, it's a bit of a strange check
but things break if I update it to the new hasChildNodes
snowystinger
changed the title
github-merge-queue bot
pushed a commit
that referenced
this pull request
Feb 12, 2025
* chore: Update TreeView API * fix docs * fix dynamic example * actually save * fix docs * review comments * Revert change to not use hasChildItems * add prop table * fix docs example * remove extra border attributes in theme * fix snapshots * fix: S2 treeview haschildnodes (#7694) * fix logic to determine hasChildNodes * revert breaking changes but use prop override * match rac logic with hook * Fix spacing regression * fix chromatic merge * fix lint
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes
Attempt at fixing #7613 (comment)
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: