-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
chore: Update TreeView API #7613
Conversation
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.
Slightly more to write but I think the composition benefits are probably worth it
children | ||
} = props; | ||
let {isDetached, isEmphasized} = useContext(InternalTreeContext); | ||
let {hasChildItems} = useContext(TreeItemContext); |
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.
hasChildRows
in the render props doesn't give you what you need?
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.
apparently i do need this right now, I'll look into it more tomorrow
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 believe it is for the case where you want to mark a TreeRow as expandable despite not having any child rows (aka async case/permissions case where the user doesn't have access to said TreeRow's content but needs to know that it is expanded)
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, was just looking into it and was scratching my head why this was needed and clearly intentional because there are tests for it 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.
Probably should be done in RAC then. This won't result in the right aria attributes being added, only the visual chevron appearing. There is a hasChildNodes
property internally, maybe that should be used instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The API update looks good to me, just some things I noticed when looking at the docs 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.
Happy for the missing props in the TreeItemContent props table to be handled in the main PR
The base branch was changed.
23a6b1a
to
04adce4
Compare
* fix logic to determine hasChildNodes * revert breaking changes but use prop override
# Conflicts: # packages/@react-spectrum/s2/src/TreeView.tsx
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, bad merge, thanks for catching. I've run chromatic on the PR now. Should be all good. |
## 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-components:TreeItemContentRenderProps TreeItemContentRenderProps {
allowsDragging?: boolean
- hasChildRows: boolean
+ hasChildItems: boolean
id: Key
isDisabled: boolean
isDragging?: boolean
isDropTarget?: boolean
isFocusVisible: boolean
isFocusVisibleWithin: boolean
isFocused: boolean
isHovered: boolean
isPressed: boolean
isSelected: boolean
level: number
selectionBehavior: SelectionBehavior
selectionMode: SelectionMode
state: TreeState<unknown>
} @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?: ReadonlyArray<any>
+ idScope?: Key
+ items?: Iterable<{}>
+} |
Closes
I think this is what https://github.com/orgs/adobe/projects/19/views/32?visibleFields=%5B%22Title%22%2C%22Assignees%22%2C%22Status%22%2C4406231%2C25365423%2C3244152%2C5195578%2C17851188%2C30557450%2C3404283%2C%22Labels%22%5D&filterQuery=&pane=issue&itemId=59197979
was asking for
The PR also removes some extraneous styles associated with borders for tree items since we aren't using them and it's better to not potentially bloat our CSS output with extra shorthands
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: