-
Notifications
You must be signed in to change notification settings - Fork 1.6k
feat: #10121 Implemented the Table Column Header Focus #10369
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
base: main
Are you sure you want to change the base?
Changes from all commits
b1a5f23
5501138
7b609f1
ee5361a
cc36fd3
2c3c225
72c324d
91f777f
cc584d0
c24df2d
67f1451
601e8cc
c429412
91ab937
8924895
97aed7c
648af4b
3ac4e25
7d8e9d8
3354253
5c41b97
fc43fae
a8a21a6
0a24d2a
87a3d2b
2cdc768
72d65a9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
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. I noticed that arrow down doesn't seem to work when you are focused on the columns now, it should move focus to the rows below
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. @LFDanLu , now the arrow down is focusing on the columns. |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -11,15 +11,46 @@ | |||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| import {getChildNodes, getFirstItem} from 'react-stately/private/collections/getChildNodes'; | ||||||||||||||||||||||||||||||||||||||||||||
| import {GridKeyboardDelegate} from '../grid/GridKeyboardDelegate'; | ||||||||||||||||||||||||||||||||||||||||||||
| import {GridKeyboardDelegate, GridKeyboardDelegateOptions} from '../grid/GridKeyboardDelegate'; | ||||||||||||||||||||||||||||||||||||||||||||
| import {ITableCollection} from 'react-stately/private/table/TableCollection'; | ||||||||||||||||||||||||||||||||||||||||||||
| import {Key, Node} from '@react-types/shared'; | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| export interface TableKeyboardDelegateOptions<T> extends GridKeyboardDelegateOptions< | ||||||||||||||||||||||||||||||||||||||||||||
| ITableCollection<T> | ||||||||||||||||||||||||||||||||||||||||||||
| > { | ||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||
| * Whether the first row or the first column header should be focused when the user tabs into the | ||||||||||||||||||||||||||||||||||||||||||||
| * table. | ||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||
| * @default 'row' | ||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||
| initialFocus?: 'row' | 'columnheader'; | ||||||||||||||||||||||||||||||||||||||||||||
|
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. Maybe instead of it being global, it should be an optional argument to getFirstKey? I have no idea if that'll be better
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. Keeping it on TableKeyboardDelegate is the safer option, and the useSelectableCollection is shared by Lists, Grids, and Trees, so it doesn’t know about initialFocus. Passing it through getFirstKey() would require changes to the shared KeyboardDelegate API and the generic hook. Keeping it as an option on the Table delegate keeps this change limited to Tables. |
||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| export class TableKeyboardDelegate<T> extends GridKeyboardDelegate<T, ITableCollection<T>> { | ||||||||||||||||||||||||||||||||||||||||||||
| private initialFocus: 'row' | 'columnheader'; | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| constructor(options: TableKeyboardDelegateOptions<T>) { | ||||||||||||||||||||||||||||||||||||||||||||
| super(options); | ||||||||||||||||||||||||||||||||||||||||||||
| this.initialFocus = options.initialFocus ?? 'row'; | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| protected isCell(node: Node<T>): boolean { | ||||||||||||||||||||||||||||||||||||||||||||
| return node.type === 'cell' || node.type === 'rowheader' || node.type === 'column'; | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| getFirstKey(fromKey?: Key, global?: boolean): Key | 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. @snowystinger brought up a good point that this may affect areas like react-spectrum/packages/react-aria/src/dnd/useDroppableCollection.ts Lines 661 to 671 in 14b74f3
and react-spectrum/packages/react-aria/src/dnd/useDroppableCollection.ts Lines 739 to 748 in 14b74f3
IMO either those call sites need to be updated accordingly to look for the first row key, or we should change this column case to specifically only trigger for normal keyboard navigation some how
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. We should have tests for these as well, or point them out if they already exist, I'm a little surprised none broke but we may not have good coverage on this
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. @LFDanLu and @snowystinger Thanks for pointing out the |
||||||||||||||||||||||||||||||||||||||||||||
| if (fromKey == null && this.initialFocus === 'columnheader') { | ||||||||||||||||||||||||||||||||||||||||||||
| let firstColumn = this.collection.columns.find( | ||||||||||||||||||||||||||||||||||||||||||||
| column => !column.props?.isDragButtonCell && !column.props?.isSelectionCell | ||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||
| if (firstColumn) { | ||||||||||||||||||||||||||||||||||||||||||||
| return firstColumn.key; | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
| return super.getFirstKey(fromKey, global); | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| getKeyBelow(key: Key, options?: {includeDisabled?: boolean}): Key | null { | ||||||||||||||||||||||||||||||||||||||||||||
| let startItem = this.collection.getItem(key); | ||||||||||||||||||||||||||||||||||||||||||||
| if (!startItem) { | ||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -34,7 +65,7 @@ export class TableKeyboardDelegate<T> extends GridKeyboardDelegate<T, ITableColl | |||||||||||||||||||||||||||||||||||||||||||
| return child.key; | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| let firstKey = this.getFirstKey(); | ||||||||||||||||||||||||||||||||||||||||||||
| let firstKey = super.getFirstKey(); | ||||||||||||||||||||||||||||||||||||||||||||
| if (firstKey == null) { | ||||||||||||||||||||||||||||||||||||||||||||
| return null; | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
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.
This is starting to worry me that we're making a breaking change
Uh oh!
There was an error while loading. Please reload this 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.
Thanks for the feedback. I looked into both points a bit more
I agree that we should avoid a breaking change! The reason I added getFirstItemKey is to make the DnD code use an actual item key. DnD expects drop target to be either item or root, so the column header returned by getFirstKey() shouldn't be passed to it, and this helper function will skip non-item nodes and find the first valid item instead.
So this supports columnheader as the initial focus for Tables without changing the shared APIs or passing an invalid key into the DnD logic.
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.
Right, but the fact that this helper is needed means that we broke an assumption on how the function worked. So even though we didn't break the api contract, there was an implicit requirement. And it's one that typescript has trouble representing, so hard to see.