Conversation
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
| if (parent) { | ||
| // siblings must exist because our original node exists | ||
| let siblings = state.collection.getChildren?.(parent.key)!; | ||
| let siblings = getDirectChildren(parent as CollectionNode<T>, state.collection as Collection<CollectionNode<T>>); |
There was a problem hiding this comment.
we used to use the getChildren method from TreeCollection but now that we've changed it to get ALL the descendants of a parent node instead of its immediate descendants, we can't use that anymore. i decided to make a new function called getDirectChildren which is essentially the same code as what getChildren was before
| let [expandedKeys, setExpandedKeys] = useControlledState( | ||
| propExpandedKeys ? convertExpanded(propExpandedKeys) : undefined, | ||
| propDefaultExpandedKeys ? convertExpanded(propDefaultExpandedKeys) : new Set(), | ||
| propExpandedKeys ? new Set(propExpandedKeys) : undefined, |
There was a problem hiding this comment.
ended up getting rid of convertExpanded because i was running into a type error where expandedKeys could either be a Set or string but since propExpandedKeys/propDefaultExpandedKeys are type Iterable<Key> and cannot be set to all, there wasn't any way expandedKeys would ever be a string
| } | ||
|
|
||
| if ((this.expandedKeys.has(node.key) || node.type !== 'item') && node.firstChildKey != null) { | ||
| return node.firstChildKey; |
There was a problem hiding this comment.
should getKeyAfter include content nodes or should it skip them?
in this case, if the node is included in expandedKeys, we'll look at its firstChildKey which will be a TreeItemContent node. however, this caused issues in DropTargetKeyboardNavigation so we check if the node is a content node and if it is, we check its nextKey. however, we could deal with that here but depends on what we want to do
|
Build successful! 🎉 |
|
Build successful! 🎉 |
LFDanLu
left a comment
There was a problem hiding this comment.
The behavior looks good, we should do some more through testing for accessibility and DnD if possible. Noted some small areas that I had some questions in
There was a problem hiding this comment.
w/ regards to rendering the drop indicator, did we test DnD with Tree sections yet? I think I remember a potential problem being the drop indicators rendered after the last row in a section and the first row in the next section and those being conflated as being the same when they aren't in practice
There was a problem hiding this comment.
yeah dnd is unlikely to work (it also doesn't work in listbox w/ sections and gridlist w/ sections). we decided for gridlist to leave it as a follow-up since we knew we were releasing it as an alpha and i think the same would apply here
There was a problem hiding this comment.
how are we releasing this as an alpha?
There was a problem hiding this comment.
i guess the package itself wouldn't say anything about it being alpha but with gridlist we just marked it as an alpha on the docs. we could also add unstable to the export
There was a problem hiding this comment.
If we do the UNSTABLE, eventually we have to remove it but also keep it around
Ok, so we're just doing it via the docs
| } | ||
| }; | ||
|
|
||
| const TreeExampleSectionRender = (args) => ( |
There was a problem hiding this comment.
I noticed in this story that the very last item is getting a posinset=4 but a setsize=2 so it seems to be counting the sections themselves for the posinset but not in the total size. Should sections be counted?
There was a problem hiding this comment.
ah right this is a similar issue to the aria-rowindex in GridList Sections that i tried to solve in a different pr when there's a mix of sections + individual items.
based on your comment here #8790 (comment) this mixed case might not be something we support but i can't remember what we resolution was in otters...so maybe this story is a bit contrived since i just copied it from another example and added section to it
but regardless, aria-posinset is calculated using node.index, so we'd need more complicated logic to correct this but the calculations for that can get expensive so we wanted to do it in the collection somehow
There was a problem hiding this comment.
that said, i have been thinking about this aria-posinset and aria-setsize and i don't think our values are quite right. it would probably take a overhaul of how we determine the index/size with each node (again going back to the accessibility issues in gridlist sections).
i think the setsize and posinset should reference rows with the same level even across sections but right now, we calculate setsize and posinset per section rather than the entire tree
what we have tho won't mess things up for current trees, just those with sections really
|
|
||
| return ( | ||
| <dom.section | ||
| {...mergeProps(DOMProps, renderProps, rowGroupProps)} |
There was a problem hiding this comment.
do we need UNSAFE or style props?
what about labelable? or global events? I'm unable to navigate to the section title, and I also don't hear it in the VO announcements for rows inside the section. So possibly global events doesn't make sense, but I assume we should be able to get labels
There was a problem hiding this comment.
you shouldn't be able to navigate to the section title. we've never had titles be focusable
There was a problem hiding this comment.
this also isn't S2 TreeView so no need for UNSAFE or style props. the renderProps should handle that i believe
There was a problem hiding this comment.
derp, i noticed that it wasn't S2 after i wrote that comment, apologies for the noise
There was a problem hiding this comment.
Outstanding question, how do we expect VO to announce Sections given that you can't get to them?
There was a problem hiding this comment.
sorry i was investigating this one a bit. for listbox sections, the section will get announced with the item. this just seems to be the supported VO behavior. but hat doesn't seem to be the case with gridlist sections/tree sections.
i made a minimal reproduction of it in this codepen: https://codepen.io/yihuiliao/pen/MYeLPMj
in comparison, here's what listbox does: https://codepen.io/yihuiliao/pen/MYeLzaJ
for me, it doesn't always announce the section it's in. it only announces the section when you change from one section to another, but better than what's happening in grids.
There was a problem hiding this comment.
im thinking about how we could get around it, maybe by passing the section/heading as an aria-labelledby to the items in it?
There was a problem hiding this comment.
might be worthwhile asking the accessibility team about this, hopefully we don't have to implement custom announcements
|
pushed up a couple changes but will get to the rest of the comments tomorrow if they are not marked as resolved or have no comments/reactions |
|
Build successful! 🎉 |
reidbarber
left a comment
There was a problem hiding this comment.
Behavior looks good! Just a few small comments:
|
Build successful! 🎉 |
snowystinger
left a comment
There was a problem hiding this comment.
pretty close, I got held up here for a while though trying to decipher between name and behaviour
|
Build successful! 🎉 |
## API Changes
react-aria-components/react-aria-components:TreeHeader+TreeHeader {
+ children?: ReactNode
+ className?: string
+ id?: string
+ render?: DOMRenderFunction<keyof React.JSX.IntrinsicElements, undefined>
+ style?: CSSProperties
+}/react-aria-components:TreeSection+TreeSection <T extends {}> {
+ aria-label?: string
+ children?: ReactNode | ({}) => ReactElement
+ className?: string
+ dependencies?: ReadonlyArray<any>
+ id?: Key
+ items?: Iterable<{}>
+ render?: DOMRenderFunction<keyof React.JSX.IntrinsicElements, undefined>
+ style?: CSSProperties
+ value?: {}
+} |
Closes
✅ Pull Request Checklist:
📝 Test Instructions:
Test RAC Tree examples both those with and without sections. It would also be good to sanity check S2 and v3 TreeView since they are built on top of RAC Tree.
🧢 Your Project: