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

makeBSS() only works with classes #9

Open
KatieK2 opened this issue Oct 13, 2014 · 9 comments
Open

makeBSS() only works with classes #9

KatieK2 opened this issue Oct 13, 2014 · 9 comments

Comments

@KatieK2
Copy link
Contributor

KatieK2 commented Oct 13, 2014

It looks like makeBSS() only works to select elements by class, and not id. This actually make sense since the slideshow is vanilla.js based (not jQuery based) but it's probably worth calling out in the documentation.

@leemark
Copy link
Owner

leemark commented Oct 14, 2014

I believe that it should work fine with id as well as class. Here's a test case: http://codepen.io/leemark/pen/zHAau

It's using querySelectorAll() so should work when passing in any valid CSS selector as the first argument. That's how it's meant to work anyway, is it not working as expected for you?

@KatieK2
Copy link
Contributor Author

KatieK2 commented Oct 14, 2014

Thanks for your test case. Here's a demo that I've been working from: http://codepen.io/KatieK2/pen/LvcyB?editors=101

After some more digging, it looks like the slide-show only works if class="bss-slides" exists on the slide container. If I remove that class from your pen, the slideshow stops working. I would have expected makeBSS() to add the appropriate class to the passed-in element.

@leemark
Copy link
Owner

leemark commented Oct 14, 2014

Excellent point, an oversight on my part! Yes, the CSS is written such that the sldieshow container has to have a particular class. Like you're saying, because the CSS relies on the container element having that particular class, the js should test for that and add it if needed.

I guess I initially wanted the flexibility of people being able to be able to modify the CSS to use whatever class name they want (as long as it's a class on the container element). But that flexibility of not having the class name hardcoded into the javascript and being able to change the class on the container and in the CSS if you choose, comes at the cost of a little bit more configuration to get the slideshow up and running (i.e. having to add class="bss-slides" onto the container element). It's a trade-off either way, but I think your suggestion is a good idea. Test for that class, add it if it doesn't exist.

Thank you!

@KatieK2
Copy link
Contributor Author

KatieK2 commented Oct 15, 2014

Glad to help - thanks for working on this cool project!

@cmolina
Copy link

cmolina commented Nov 20, 2014

I think you should edit the Getting started example in README.md to make explicit that people should include the bss-slides class. Nice work :)

@ezwerk
Copy link

ezwerk commented Oct 29, 2015

I may be running into a related issue as my images disappear and nothing is happening.
http://codepen.io/ezwerk/pen/MaGZqY

Any input would be greatly appreciated.

@leemark
Copy link
Owner

leemark commented Oct 29, 2015

Hi @ezwerk - in your codepen the link to the js file is not working. Line 20 in the html. When I change the src to point to this js file instead it starts working: http://leemark.github.io/better-simple-slideshow/js/better-simple-slideshow.min.js

@ezwerk
Copy link

ezwerk commented Oct 30, 2015

That was it! Sheesh... had the folders mismatched. Thanks for catching that and also for getting back to me so quick!

Greatly appreciated!

@leemark
Copy link
Owner

leemark commented Oct 30, 2015

@ezwerk no problem, good luck! 👍

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

No branches or pull requests

4 participants