-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat: Add support for nested table rows #9740
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
Changes from all commits
eee43b1
db18a02
dca2c50
e9504f8
3e22876
4d65d03
df4fce6
dc3a36c
2370433
e2eb072
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 |
|---|---|---|
|
|
@@ -12,7 +12,6 @@ | |
|
|
||
| import {DOMAttributes, RefObject} from '@react-types/shared'; | ||
| import {GridRowProps} from '@react-aria/grid'; | ||
| import {tableNestedRows} from '@react-stately/flags'; | ||
| import {TableState} from '@react-stately/table'; | ||
|
|
||
| export interface TableHeaderRowAria { | ||
|
|
@@ -32,7 +31,7 @@ export function useTableHeaderRow<T>(props: GridRowProps<T>, state: TableState<T | |
| role: 'row' | ||
| }; | ||
|
|
||
| if (isVirtualized && !(tableNestedRows() && 'expandedKeys' in state)) { | ||
| if (isVirtualized && state.treeColumn == null) { | ||
|
Member
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. bit of a strange check, could we put it behind some other state property? like
Member
Author
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. hmm why is it strange? if there is a tree column it is a tree grid.
Member
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. The name just doesn't really make sense in my head. Looking at it again today just reviewing my comments and closing out the answered ones, I have no idea what this check is for. |
||
| rowProps['aria-rowindex'] = node.index + 1; // aria-rowindex is 1 based | ||
| } | ||
|
|
||
|
|
||
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.
when does this happen? was it a bug we had not keeping the id as a key?
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.
Nested rows without an explicit id would have an idScope prepended for every level. Since the collection is flattened anyways I suppose it makes sense to skip that.
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 this was meant to scope the ids of the cells within a row so you can loop over the columns multiple times and not get duplicates. We don't want that for rows. If an explicit
idis given, we should respect that. That was the change here. I was also considering moving this scoping intoCellitself so it would only apply there. wdyt?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 think moving this into Cell only makes sense, I can't really think of any other cases where we'd also want to do this scoping
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.
will have to think about how to do this.
createLeafComponentdoesn't really provide a way to add that logic right now. maybe we could do it by creating a wrapper component or something.