Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

carousel js does not read data-* options on initialization #5279

Closed
ratbeard opened this Issue Sep 25, 2012 · 9 comments

Comments

Projects
None yet
7 participants

Hello. From the docs[1]:

Options can be passed via data attributes or JavaScript. For data attributes, append the option name to data-, as in data-interval="".

I do not see this implemented in the source [2]. Only default values and passed in js options are used. We would like this for data-interval.

I can submit a patch to fix this. I presume I should implement it the same as modal[3], and just grab all potential data properties off the object (not grab only 'interval' and 'pause')?

[1] http://twitter.github.com/bootstrap/javascript.html#carousel
[2] https://github.com/twitter/bootstrap/blob/master/js/bootstrap-carousel.js#L146
[3] https://github.com/twitter/bootstrap/blob/master/js/bootstrap-modal.js#L203

JS FIDDLE THAT SHOWCASES THE PROBLEM:

http://jsfiddle.net/vm9vX/1/

Hey @ratbeard,

Thanks for opening this issue! Unfortunately, it looks like it fails to pass the criteria neccessary for submitting to bootstrap. The following things are currently failing:

  • should include a jsfiddle/jsbin illustrating the problem if tagged with js but not a feature

For a full list of issue filing guidelines, please refer to the bootstrap issue filing guidelines.

thanks!

ratbeard commented Oct 8, 2012

Umm. I edited the original bug to include the jsfiddle link that was in my comment as I'm guessing thats what tripped up this little automated script.

I made the 1 line fix but got a bit bogged down getting my homebrew/node/npm modules up to date to be able to write some tests, hopefully will have time to complete that and submit a patch.

Owner

fat commented Oct 18, 2012

hm you shouldn't need to install anything to run the tests. just open js/tests/index.html

Owner

fat commented Oct 18, 2012

i think this was fixed though – there was a copy paste error in the carousel

@fat fat closed this Oct 18, 2012

@ghost

ghost commented Nov 27, 2012

This doesn't look to have been updated in the minified version. Hard to tell as it's minified, but it's not responding to it.

I still experience this problem as well, not even in the minified version.

Contributor

Yohn commented Dec 23, 2012

I think I found a fix for it

EDITED::
I previously posted a fix here, but I found a different / simpler way to fix it, and sent a pull request in for it.. #6382

Hi

This bug still exists!!!

The pull request

twitter#6382

got reopend because tests were failing but after the fix it only got 'closed' but not 'merged'!

This specific change

Yohn/bootstrap@7688770

fixes it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment