Skip to content

allow default TableRow keypress event behavior (#211) #276

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.

Sign up for GitHub

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

Merged
jeroenransijn merged 1 commit into from
Aug 13, 2018
Merged

allow default TableRow keypress event behavior (#211) #276

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion src/table/src/TableRow.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,21 @@ export default class TableRow extends PureComponent {
}
}

onRef = node => {
this.node = node
}

handleKeyPress = e => {
if (this.props.isSelectable) {
if (e.key === 'Enter' || e.key === ' ') {
this.props.onSelect()
e.preventDefault()
Copy link
Contributor

@mshwery mshwery Aug 6, 2018

Choose a reason for hiding this comment

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

Nice! Should we find another way to allow consumers to opt-in to preventing scroll of the page upon these key presses?

Copy link
Contributor Author

@JustAboutJeff JustAboutJeff Aug 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@mshwery Thanks for the feedback and additional context on the original implementation here. Appreciate it!

As an alternative to additional configuration for opt-in behavior, it occurred to me that we could instead continue to invoke preventDefault but first wrap the function call within a conditional that would check if the event target is in fact the TableRow itself.

This way, if consumers of 🌲 decide to nest additional UI that can take focus or accept user input within an isSelectable TableRow, the browser can and will scroll and direct focus to these child UI elements when the event originates there. This is good, and helps with web accessibility standards. This is the scenario captured from the original issue report here.

Alternatively, for an isSelectable TableRow that contains text or other children that do not provide a target to fire a keyPress event, preventDefault will be called and browser scrolling will not occur when selecting a row. This scenario is captured in the existing storybook example here


// NOTE: In scenarios where the target for key press is our
// tabIndex <Pane> below, prevent the default event action
// so that browser scroll will not occur.
if (e.target === this.node) {
e.preventDefault()
}
}
}

Expand All @@ -83,6 +93,7 @@ export default class TableRow extends PureComponent {

return (
<Pane
innerRef={this.onRef}
display="flex"
{...(isSelectable
? {
Expand Down
11 changes: 11 additions & 0 deletions src/table/stories/index.stories.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,17 @@ storiesOf('table', module)
<TableRow>TableRow</TableRow>
</Box>
))
.add('TableRow isSelectable with nested UI', () => (
<Box padding={40}>
{(() => {
document.body.style.margin = '0'
document.body.style.height = '100vh'
})()}
<TableRow isSelectable>
<input type="text" placeholder="type a space" />
</TableRow>
</Box>
))
.add('TableHeaderCell', () => (
<Box padding={40}>
{(() => {
Expand Down