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.

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

Jump to bottom

SafeAnchor a11y - Provide keydown handler for "space" #2697

Merged
merged 5 commits into from Aug 26, 2017
Merged

SafeAnchor a11y - Provide keydown handler for "space" #2697

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2f88092
Provide keydown handler for "space"
Jul 10, 2017
aac3d06
Check for key, not keycode
Jul 10, 2017
831dbd2
Only add handler if rendering `a` tag
Jul 10, 2017
b8800db
Prevent default only for space, don't check for `a`
Jul 11, 2017
b297dd5
Style nits
taion Jul 11, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 13 additions & 1 deletion src/SafeAnchor.js
View file
Open in desktop
Expand Up @@ -2,9 +2,12 @@ import React from 'react';
import PropTypes from 'prop-types';
import elementType from 'prop-types-extra/lib/elementType';

import createChainedFunction from './utils/createChainedFunction';

const propTypes = {
href: PropTypes.string,
onClick: PropTypes.func,
onKeyDown: PropTypes.func,
disabled: PropTypes.bool,
role: PropTypes.string,
tabIndex: PropTypes.oneOfType([
Expand Down Expand Up @@ -36,6 +39,7 @@ class SafeAnchor extends React.Component {
super(props, context);

this.handleClick = this.handleClick.bind(this);
this.handleKeyDown = this.handleKeyDown.bind(this);
}

handleClick(event) {
Expand All @@ -55,8 +59,15 @@ class SafeAnchor extends React.Component {
}
}

handleKeyDown(event) {
if (event.key === ' ') {
event.preventDefault();
this.handleClick(event);
}
}

render() {
const { componentClass: Component, disabled, ...props } = this.props;
const { componentClass: Component, disabled, onKeyDown, ...props } = this.props;

if (isTrivialHref(props.href)) {
props.role = props.role || 'button';
Expand All @@ -74,6 +85,7 @@ class SafeAnchor extends React.Component {
<Component
{...props}
onClick={this.handleClick}
onKeyDown={createChainedFunction(this.handleKeyDown, onKeyDown)}
/>
);
}
Expand Down
11 changes: 11 additions & 0 deletions test/SafeAnchorSpec.js
View file
Open in desktop
Expand Up @@ -39,6 +39,17 @@ describe('SafeAnchor', () => {
handleClick.should.have.been.calledOnce;
});

it('provides onClick handler as onKeyDown handler for "space"', () => {
const handleClick = sinon.spy();

tsp(<SafeAnchor onClick={handleClick} />)
.shallowRender()
.find('a')
.trigger('keyDown', { key: ' ', preventDefault() {} });

handleClick.should.have.been.calledOnce;
Copy link
Contributor

Choose a reason for hiding this comment

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

expect(handleClick).to.have.been.calledOnce ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They're equivalent, I was following the pattern of the "it forwards onclick" test https://github.com/andy-j-d/react-bootstrap/blob/b8800dbba51e86ed4bdd34c046f820a67194029c/test/SafeAnchorSpec.js#L39

Copy link
Member

Choose a reason for hiding this comment

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

yeah this is fine as long as we're not using assert – i prefer expect but consistency has some value

});

it('prevents default when no href is provided', () => {
const handleClick = sinon.spy();

Expand Down