Issue #3731 #12016 carousel keyboard rebased

[francis-shuoch]

As @XhmikosR said, I've rebased this branch #12052 , squashed old 3 commits, and simplified the code as @fat kindly pointed out. But since I got rebase conflicts (the docs is split into separated files), I ended up a new PR .

[francis-shuoch]

a demo can be seen here : http://jsbin.com/eneRULi/11

[saas786]

I just viewed this http://jsbin.com/eneRULi/11
@francis-liberty and its showing several errors in console,

Failed to load resource: the server responded with a status of 404 (Not Found) http://jsbin.com/dist/css/bootstrap.css
Failed to load resource: the server responded with a status of 404 (Not Found) http://jsbin.com/eneRULi/carousel.css
Failed to load resource: the server responded with a status of 404 (Not Found) http://jsbin.com/dist/js/bootstrap.min.js
Failed to load resource: the server responded with a status of 404 (Not Found) http://jsbin.com/docs-assets/js/holder.js
event.returnValue is deprecated. Please use the standard event.preventDefault() instead.
Uncaught TypeError: Cannot read property 'end' of undefined 

so please fix this and I would like to test this keyboard support and give my feedback.

thanks.

[hnrch02]

@saas786 Here you go.

[francis-shuoch]

@hnrch02 yeh, thanks for your help. And also @saas786 , this is the updated version, with the simplifications suggested by fat. http://jsbin.com/eneRULi/32

[fat]

could you squash these @francis-liberty ? thanks!

[fat]

could you line these lines up with the first line… so the ? etc are all lined up? would appreciate it :)

[francis-shuoch]

@fat You mean like this? Of course I can do this.

var type = e.which == 37 ? 'prev'  :
           e.which == 38 ? 'first' :
           e.which == 39 ? 'next'  :
           e.which == 40 ? 'last'  : false

[fat]

yes please, thanks

[cvrebert]

@fat Just to confirm: So aside from the squashing and that one style issue, you'd be okay with merging this, correct?

[fat]

this is kinda expensive… basically on all keydowns we now have to query the entire dom… checking every elements for a data attribute… not the quickest expression.

might be better to have this in the carousel instance…

[francis-shuoch]

@fat How about

$('[data-ride="carousel"]').on('keydown', function (e) {

[fat]

that doesn't effect carousels added after load, so that's not really good either

[fat]

@cvrebert pretty much… slightly concerned about the global key listener. kinda rough hit to anyone using bootstrap for a feature that probably wont be taken advantage of that much.

Might be better to tie it directly into the carousel instance and have it only setup on instantiation

[cvrebert]

@fat Ah. I was/am just looking for places where I can speed up the PR process by rebasing and fixing minor style issues for folks, thus avoiding some back-and-forth delay.

[francis-shuoch]

@fat I agree with your concern, I can change and make a new commit, and also that line-align style change. @cvrebert You can kindly check it later.

[saas786]

@francis-liberty good work, I like it now.

As @fat pointed out, its better if we avoid global initiation. I would suggest if we go optional way, I mean if we make it optional and only execute if user pass parameters for keyboard support like

https://github.com/woothemes/FlexSlider/blob/master/jquery.flexslider.js#L1090
does,
also
https://github.com/woothemes/FlexSlider/blob/master/jquery.flexslider.js#L1091

this is best way (in my opinion) for this feature.

and this kind of conditional implementation is recommended by me.

https://github.com/woothemes/FlexSlider/blob/master/jquery.flexslider.js#L96

[cvrebert]

Per our code style guide, please use double-quotes in HTML.

[francis-shuoch]

@cvrebert I read your links, just to make it clear, you mean something like this? (of course I can move the big block of keydown event function outside the constructor, I just want to make a point here.)

  var Carousel = function (element, options) {
    this.$element    = $(element)
    this.$indicators = this.$element.find('.carousel-indicators')
    this.options     = options
    this.paused      =
    this.sliding     =
    this.interval    =
    this.$active     =
    this.$items      = null

    this.options.pause == 'hover' && this.$element
      .on('mouseenter', $.proxy(this.pause, this))
      .on('mouseleave', $.proxy(this.cycle, this))

    this.options.keyboard && $(document)
      .on('keydown', '[data-ride="carousel"]', function (e) {
        // get the direction based on keyboard input
        var type = e.which == 37 ? 'prev'  :
               e.which == 38 ? 'first' :
               e.which == 39 ? 'next'  :
               e.which == 40 ? 'last'  : false

        if (!type) return // exits if not a type we care about

        // only accept arrow left, right, top and down.
        // default event behavior is prevented for these four keys.
        // other keyboard input is not affected
        var $carousel = $(document.activeElement)

        $carousel
          .carousel('pause')
          .carousel(/next|prev/.test(type) ? type : type == 'first' ? 0 : $(this).find('.item').length - 1)
          .carousel('cycle')

        e.preventDefault()
      })
  }

[cvrebert]

@francis-liberty I'm not sure what links you're referring to. I had no feedback to offer regarding your JS.

[francis-shuoch]

@cvrebert @saas786 Oh, sorry, I hadn't made it clear. I'm referring to saas786 and fat 's concern about the global key listener $(document).on('keydown', '[data-ride="carousel"]'..... Look up the latest four comments and you find what I'm talking about.

[saas786]

@francis-liberty yes something like that, but outside of main constructor (better if you can make it similar to Flexslider as per my links above, keyboard feature).

[fat]

closing in favor to this pr: #13787

thanks for the work @francis-liberty!