Skip to content
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

feat: cell-level keyboard navigation for Table #1211

Merged
merged 23 commits into from
Sep 24, 2020

Conversation

jacobdevera
Copy link
Contributor

@jacobdevera jacobdevera commented Sep 16, 2020

Description

Pending update of fundamental-styles to incorporate this PR SAP/fundamental-styles#1661 to hide screen reader instructions

  • add Table example that uses interactive content (similar to https://openui5.hana.ondemand.com/entity/sap.ui.table.Table/sample/sap.ui.table.sample.Basic)
  • add cell-level keyboard navigation
    • arrow keys to move between cells; if there is one focusable element within the field without any arrow-key handling, it will take focus
    • Enter to toggle 'edit mode': in edit mode, all tabbable fields can be navigated to with the Tab key
    • Esc to exit edit mode
  • add compact, condensed, keyboardNavigation, and localizedText props
  • use aria-live region for announcing table status

Out of Scope / Known Issues (to be created)

  • row-level keyboard navigation
  • Select should be full-width in cells (likely requires prop passing by putting into a FormGroup to apply full-width styling)
  • DatePicker popover when opened from a cell appears to gradually shift off-screen when changing months; this doesn't happen with the DatePicker alone

@jacobdevera jacobdevera self-assigned this Sep 16, 2020
@netlify
Copy link

netlify bot commented Sep 16, 2020

Deploy preview for fundamental-react ready!

Built with commit f65aedd

https://deploy-preview-1211--fundamental-react.netlify.app

@jacobdevera jacobdevera marked this pull request as ready for review September 18, 2020 21:50
className={tableClasses}
ref={tableRef}
role={keyboardNavigation ? 'grid' : 'table'}>
<caption aria-live='polite' className='fd-table__instructions'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is __instructions intentional? In fd-styles the class for <caption> appears as fd-table__caption. Currently the text is visible when traversing through the cells.

Copy link
Contributor Author

@jacobdevera jacobdevera Sep 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to change it to __caption in the end, but didn't put that change here yet. Thanks for catching that.

Copy link
Member

@prsdthkr prsdthkr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some a11y stuff:

src/Table/__stories__/Table.stories.js Outdated Show resolved Hide resolved
src/Table/__stories__/Table.stories.js Outdated Show resolved Hide resolved
src/Table/__stories__/Table.stories.js Outdated Show resolved Hide resolved
Copy link
Member

@prsdthkr prsdthkr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some missing props and typos.

src/Table/Table.js Outdated Show resolved Hide resolved
src/Table/__stories__/Table.stories.js Outdated Show resolved Hide resolved
src/Table/Table.js Outdated Show resolved Hide resolved
Copy link
Member

@prsdthkr prsdthkr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🥇

Copy link
Contributor

@Mike-Diaz Mike-Diaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Keyboard nav works as expected!

@jacobdevera jacobdevera merged commit 2f82eef into master Sep 24, 2020
@jacobdevera jacobdevera deleted the feat/table-navigation branch September 24, 2020 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants