ListView updates #2238
ListView updates #2238
Conversation
jluyau
requested review from
dannify,
devongovett and
snowystinger
as code owners
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
TODO:
|
| <Item textValue="row1"> | ||
| <Text>row 1</Text> | ||
| <Text slot="description">description for row 1</Text> | ||
| {renderActions({onPress: () => console.log('row 1')})} |
There was a problem hiding this comment.
Wonder if we need a way to make the actions in non-folder items line up with actions in folder items? Right now, they don't because folders have the chevron. I guess we'd either always reserve space for the chevron, or somehow detect if any of the items in the list are folders and only do it then. But that might be weird if you scrolled down and loaded more items and everything shifted when a folder came in.
There was a problem hiding this comment.
is it unreasonable to let it behave the way you say here (which is how subgrids would behave in the same circumstance) and they'd auto adjust when we detect a folder. Then have a prop that says I may have some folders where we'd reserve the space ahead of time so we don't get the shifting?
There was a problem hiding this comment.
Chatted about this with Alan, and he mentioned that if an item was a parent item, then the action buttons wouldn't line up. So, this is expected?
…ain components, add prop to actionmenu
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Added @LFDanLu 's code for cell selection |
|
Next listview alpha:
|
| @@ -2,5 +2,6 @@ | |||
| "selectedCount": "{count, plural, =0 {No items selected} one {# item selected} other {# items selected}}.", | |||
| "selectedAll": "All items selected.", | |||
| "selectedItem": "{item} selected.", | |||
| "deselectedItem": "{item} not selected." | |||
| "deselectedItem": "{item} not selected.", | |||
| "select": "Select" | |||
There was a problem hiding this comment.
Should we move over all the translations too?
| @@ -74,7 +91,8 @@ function ListView<T extends object>(props: ListViewProps<T>, ref: DOMRef<HTMLDiv | |||
| }), [collection]); | |||
| let state = useGridState({ | |||
| ...props, | |||
| collection: gridCollection | |||
| collection: gridCollection, | |||
| allowsCellSelection: true | |||
There was a problem hiding this comment.
I'm surprised this works... It looks like the row element is still getting aria-selected rather than the cell element, which is fine, but kinda surprising given this option.
There was a problem hiding this comment.
yeah, that option is just to make this block fire so we get the cell's key rather than the row's key when we do any kind of selection toggling/change. One alternative is to have a custom onAction on the row that does manager.toggleSelection(childkey) or something like that
There was a problem hiding this comment.
but if the cell's key is being used, then how does aria-selected end up on the row?
There was a problem hiding this comment.
was playing around getting CardView working with a selection bug going from "all" to a subset of items, another possible solution is to change the types here
react-spectrum/packages/@react-spectrum/list/src/ListView.tsx
Lines 67 to 71 in 562b400
to
type: 'blah' (aka something that isn't item),
childNodes: [{
...item,
index: 0,
type: 'item'
This means we can get rid of allowsCellSelection since the cells are now the 'item` in SelectionManager logic
There was a problem hiding this comment.
@devongovett sorry didn't refresh, just saw your other comment. The way aria-selected ends up in the right place is that the node provided to ListItem's useGridRow call is NOT the grid row node from the GridCollection but rather the node from ListCollection from the useListState call since we pass the ListCollection to the Virtualizer, not the GridCollection.
Those nodes from the ListCollection have matching keys with the cell because we generate the grid cells from the ListCollection:
react-spectrum/packages/@react-spectrum/list/src/ListView.tsx
Lines 66 to 73 in 562b400
There was a problem hiding this comment.
Oh I see. Well this is fine for now, but we can test a11y and see what is better at some point.
|
Build successful! 🎉 |
jluyau
force-pushed
the
listview-alpha3
branch
from
85d4949
to
3edf3e0
Compare
|
Build successful! 🎉 |
ListView:
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: