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

Add keyboard option to carousel #14590

Merged
merged 1 commit into from
Oct 3, 2014
Merged

Add keyboard option to carousel #14590

merged 1 commit into from
Oct 3, 2014

Conversation

hnrch02
Copy link
Collaborator

@hnrch02 hnrch02 commented Sep 10, 2014

Also adds unit tests for keyboard events.

Fixes #14468.

@hnrch02 hnrch02 added the js label Sep 10, 2014
@hnrch02
Copy link
Collaborator Author

hnrch02 commented Sep 10, 2014

@cvrebert Docs wording LGTY?

@hnrch02 hnrch02 force-pushed the carousel-keyboard-option branch 2 times, most recently from be76960 to efb13de Compare September 10, 2014 21:35
@cvrebert
Copy link
Collaborator

👍

@hnrch02
Copy link
Collaborator Author

hnrch02 commented Sep 16, 2014

/to @fat

Also adds unit tests for keyboard events.

Fixes #14468.
@hnrch02 hnrch02 force-pushed the carousel-keyboard-option branch from efb13de to 038a63b Compare October 3, 2014 03:16
@fat
Copy link
Member

fat commented Oct 3, 2014

I like to think of the plugins like an ecosystem – and if we add this option in one place, we should add it to all the plugins…

That said, I'm not really crazy about adding this keyboard option. Seems like a bit of a weird use case – and something that could be solved in other ways.

@fat
Copy link
Member

fat commented Oct 3, 2014

If i'm missing some other perspective though, let me know :) - the code looks really good, and swooned at the unit tests

@hnrch02
Copy link
Collaborator Author

hnrch02 commented Oct 3, 2014

I agree that the specific use case mentioned in #14468 is weird but I feel that this is a small change that helps out somebody which is entirely relying on the data-api. Besides that we have a keyboard option for modals, removing the escape listener for modals would be just as easy as the workarounds you suggested in #14468.

@fat
Copy link
Member

fat commented Oct 3, 2014

oh yeah… hrm… it does still really feel wrong to have an option that is essentially fuck accessibility, but your call.

can't remember the exat details of the modal keyboard option, but i think i was also not super excited about it

fat added a commit that referenced this pull request Oct 3, 2014
@fat fat merged commit 2c562d2 into master Oct 3, 2014
@fat fat deleted the carousel-keyboard-option branch October 3, 2014 06:11
@fat
Copy link
Member

fat commented Oct 3, 2014

merged it… i think we should audit in 4 and see if we can't have a more graceful way of turning these types of things off across plugins tho

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Carousel - is it possible to turn off keyboard navigation?
3 participants