feat(Table): Adding table navigation #2147
Conversation
kolaps33
requested review from
dvdzkwsk,
dzearing,
ecraig12345,
JasonGore,
jdhuntington,
kenotron,
layershifter,
levithomason,
miroslavstastny and
mnajdova
as code owners
| moreOptionButton, | ||
| ], | ||
| onClick: () => handleRowClick(1), | ||
| 'aria-label': 'custom text', |
There was a problem hiding this comment.
what does custom text mean? I think it would be better to use meaningful labels in the examples
|
can you please add |
| @@ -0,0 +1,22 @@ | |||
| import { Accessibility } from '../../types' | |||
| import { IS_FOCUSABLE_ATTRIBUTE } from '../../attributes' | |||
| // import * as keyboardKey from 'keyboard-key' | |||
There was a problem hiding this comment.
remove these comments
| import * as keyboardKey from 'keyboard-key' | ||
| import gridHeaderCellBehavior from './gridHeaderCellBehavior' | ||
|
|
||
| // add key actions unit tests |
| // add key actions unit tests | ||
|
|
||
| /** | ||
| * @description |
There was a problem hiding this comment.
if you don't add anything to description just remove them, otherwise add description about the behavior.
| @@ -609,6 +609,21 @@ definitions.push({ | |||
| }, | |||
| }) | |||
|
|
|||
| // Triggers 'unsetRowTabbable' action using SHIFT + TAB key on 'root'. | |||
There was a problem hiding this comment.
Maybe we should abstract this, so we can include stuff like Triggers 'openAndFocusFirst' action with 'altKey'+'ArrowDown' on 'root' slot.
Maybe we can also add the part with based on the prop $propname so we ca include stuff like:
...(!props.open && {
openAndFocusFirst: {
keyCombinations: [{ keyCode: keyboardKey.ArrowDown, altKey: true }],
},
}),| @@ -513,16 +513,16 @@ export default class FocusZone extends React.Component<FocusZoneProps> implement | |||
|
|
|||
| const doc = getDocument(this._root.current) | |||
|
|
|||
| if (this.props.onKeyDown) { | |||
There was a problem hiding this comment.
you can probably use lodash invoke here: _.invoke(this.props, 'onKeyDown', ev)
There was a problem hiding this comment.
as this is FocusZone code I rather don't want to change the code...
Even this change we are asking if focusZone people agree with the change.
| Choose desired accessibility behavior for grid cell depending on the use case.(Check the{' '} | ||
| {link('Behaviors', '/components/table/accessibility')} section). | ||
| </Text>, | ||
| 'Provide label to the Table component using `aria-label` or `aria-labelledby` prop.', |
There was a problem hiding this comment.
pls add: If no label is provided, the whole header will be read by the screen reader when the user navigates to the table
| {link('Behaviors', '/components/table/accessibility')} section). | ||
| </Text>, | ||
| 'Provide label to the Table component using `aria-label` or `aria-labelledby` prop.', | ||
| 'Provide label to the Row component using `aria-label` or `aria-labelledby` prop. If not, then each cell of the row is narrated by screen reader.', |
There was a problem hiding this comment.
pls change to Consider providing label to the TableRow component using aria-label or aria-labelledby prop. When not provided, the whole row (all cells) will ne narrated by the screen reader when the user navigates to the row.
| 'Provide label to the Table component using `aria-label` or `aria-labelledby` prop.', | ||
| 'Provide label to the Row component using `aria-label` or `aria-labelledby` prop. If not, then each cell of the row is narrated by screen reader.', | ||
| 'Provide label to the table header column, if cell has no content.', | ||
| 'Stop event propagation, when you will add actionable element into the grid cell.', |
There was a problem hiding this comment.
If you use actionable element or component in the cell, stop propagation of the click event if you do not want the interacion with the element to perform row click action.
| ] | ||
|
|
||
| const dontList = [ | ||
| "Don't set onClick action on row, which will be not available in the cell of grid, as well. With screen reader navigation user can navigate only cell by cell.", |
There was a problem hiding this comment.
If you use onClick on the row, make also one (preferably the first) cell actionable and add onClick handler with the same action to it. Screen readers do not navigate to rows, so actions need to be available on cells as well.
|
|
||
| function tagButton(tagName: string) { | ||
| return ( | ||
| <Provider |
There was a problem hiding this comment.
make sure to have only one provider in the example, not for each button
Adding table navigation like:
What should works:
Microsoft Reviewers: Open in CodeFlow