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

Adds option sliderTagName to allow for specifiying HTML element of the slide wrapper #687

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

jeryj
Copy link

@jeryj jeryj commented Feb 2, 2018

I ran into an issue where I wanted the individual slider items to be <li>s, but the .flickity-slider is set to be a <div> with no option to change it. I needed .flickity-slider to be a <ul> for valid HTML.

This PR introduces the option sliderTagName to pass the tagName you want to use for your .flickity-slider so you can use any tagName you want.

@desandro
Copy link
Member

desandro commented Feb 9, 2018

Thanks for opening this PR. It's a neat addition, but I'm not sure how useful it is. The slider is not necessarily a good list item, as there is only ever one of them in the Flickity instance.

Anyways, I'll keep this PR open for a bit to see if there's any other interest.
Add a 👍 reaction to this issue if you would like to see this feature added. Do not add +1 comments — They will be deleted.

@jeryj
Copy link
Author

jeryj commented Feb 9, 2018

@desandro My intention was for the slider to be a <ul> or <ol> and have the slider children be <li>s. I agree that the slider wrapper being an <li> would be questionable :)

On a site I'm working on we're displaying trending articles in a Flickity slider. Having the slide wrapper be an <ol> with each slide an <li> would be the ideal HTML. This PR would allow the slider to have HTML like:

<ol class="flickity-slider">
    <li class="trending-post">....</li>
    <li class="trending-post">....</li>
    <li class="trending-post">....</li>
</ol>

@desandro
Copy link
Member

desandro commented Feb 9, 2018

ah! right, thanks for clearing that up. Yeah, that semantically makes sense.

@AndreMarquesDev
Copy link

Was this ever added as an option to Flickity?

@tklives
Copy link

tklives commented Aug 12, 2019

I agree - I would love to see this feature get merged in. I have a component that I am building that has somewhat painted me in a corner as far as needing to use <ul>\<li> as the flickity slider. It would be great to have this additional flexibility!

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

Successfully merging this pull request may close these issues.

4 participants