-
Notifications
You must be signed in to change notification settings - Fork 629
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
BUGFIX - updates to arrow key navigation handler to work in IE/Edge. … #201
Conversation
…Changed match from e.key => e.keyCode. Updated Carousel tests to account for new methods. There are two tests that fail, but they were failing in the existing repo as well.
Hey @jzeltman, which 2 tests are failing for you? I just tried and everything passes here.
|
src/components/Carousel.js
Outdated
@@ -225,12 +225,12 @@ class Carousel extends Component { | |||
const { axis } = this.props; | |||
const isHorizontal = axis === 'horizontal'; | |||
|
|||
const nextKey = isHorizontal ? 'ArrowRight' : 'ArrowDown'; | |||
const prevKey = isHorizontal ? 'ArrowLeft' : 'ArrowUp'; | |||
const nextKey = isHorizontal ? 39 : 40; |
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.
Could you create a map to identify these numbers with a readable name?
example:
const keyNames = {
ArrowRight: 39,
ArrowDown: 40
};
const nextKey = isHorizontal ? keyNames.ArrowRight : keyNames.ArrowDown;
src/__tests__/Carousel.js
Outdated
|
||
expect(componentInstance.increment.mock.calls.length).toBe(1); | ||
expect(componentInstance.decrement.mock.calls.length).toBe(0); | ||
}); | ||
|
||
it('should call only decrement on ArrowUp', () => { | ||
componentInstance.navigateWithKeyboard({key: 'ArrowUp'}); | ||
it('should call only decrement on 38', () => { |
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.
Please, keep the tests descriptions as "should call only decrement on ArrowUp (38)"... Much easier to understand.
Thanks @jzeltman, it was published to npm as 3.1.30 |
This fixes #200 |
Changed match from e.key => e.keyCode. Updated Carousel tests to account for new methods.
There are two tests that fail, but they were failing in the existing repo as well.