viktorlarsson / vue-tiny-slider

Vanilla javascript slider for all purposes created by ganlanyuan in Vue.
MIT License
136 stars 51 forks source link

v-on events do not bind to elements cloned for loop #16

Open strangemethod opened 6 years ago

strangemethod commented 6 years ago

In my slider, each slide has a button that is bound to an event using v-on:click. When I set :loop="true" the slides are cloned in order to create the infinite loop. This appears to happen after the component has rendered, and as a result the cloned elements are not bound to the click event.

Here is a simple example: https://jsfiddle.net/dfg4wt9s/5/

If you click previous to view the cloned "last" slide, its button will not be clickable. Is there a way to bind events to the cloned elements after the slider has initiated without resorting to DOM manipulation?

viktorlarsson commented 6 years ago

I'll see what I can do. Any ideas here @rymdkapten ?

strangemethod commented 6 years ago

Thanks @viktorlarsson. Since the slides are cloned using the cloneNode()method in the vanilla tiny-slider.js, it seems to me that dom manipulation in the mounted() method may likely be the only way to bind events to those slides. Feel free to close if there is no other reasonable solution.

gladishukD commented 6 years ago

Hi! how to solve a loop problem?

JGriffCS commented 6 years ago

@viktorlarsson

This issue has been inactive for a while so I'm not sure how much benefit this will be at this point, but I ran up against the same issue while writing my own wrapper for tiny-slider. I'm not sure if the method is ideal, but I got around the issue by moving the cloning logic into my Vue wrapper and eliminating the need for tiny-slider to create any clones.

To accomplish this I did the following: 1) Copy the logic from getCloneCountForLoop and getItemsMax into the Vue wrapper. 2) Create a function to deep clone Vue nodes. I can't seem to find the example I used at the moment, but I ended up having to make a few small modifications to the cloned node's key, when present, to avoid duplicate key errors (in the case where the slider contents are generated by a v-for). 3) In the wrapper's render function, prior to creating the slider element, create an array to store the cloned slides and get the required number of clones by calling getCloneCountForLoop with this.$slots.default.length. 4) Deep clone this.$slots.default based on the number returned by getCloneCountForLoop and push/concat the clones into the clonedSlides array. 5) Render the wrapper using both the slots and cloned slides:

render(h) {
    return h('div', [
        this.$slots.default,
        ...clonedSlides,
    ]);
}

I haven't tested with all possible use cases (my wrapper is simpler because we don't need to support the full option set), but this should result in the getCloneCountForLoop function inside of tiny slider to always return 0 because enough slides already exist to support looping. Since the cloned slides are Vue nodes their lifecycle hooks, events, etc... should all function as normal.

If you'd like I can fork the repo and open a PR.

viktorlarsson commented 6 years ago

Hi @JGriffCS ,

Thanks for your contribution, I would love if you could fork it and create a PR!

viktorlarsson commented 5 years ago

Did you get a chance to create a PR @JGriffCS ?

lukas-schaetzle commented 4 years ago

I would also absolutely love to see a fix for this. Maybe I try to follow @JGriffCS instructions by myself but I'm not sure if I want to invest the needed time in this as I've already needed way more time than expected to find a suited Slider which works for my needs an this component also suffers from the not working center mode which I would need, too...

JGriffCS commented 4 years ago

@viktorlarsson

Apologies for falling off the face of the earth. After I posted my original findings here I attempted to fork and replicate my fix, but bumped into some issues that I wasn't able to resolve and ended up having to shelve the change. I ended up getting bogged down in other work and never had a chance to revisit it. Tummerhore's ping has put this back on my radar though, and in the spirit of Hacktoberfest I'm giving it another shot.

I think I've managed to work around the issues I was facing during my last attempt, but I'm still running through all the various test cases that result in different cloning behavior to ensure that there are no holes in my logic. It's not the most elegant solution, as it involves duplicating functionality from tiny-slider, but I was unable to come up with any other solution for persisting events outside of duplicating the cloning logic in Vue.

I'll update here and open a PR if my attempt ends up being successful.

JGriffCS commented 4 years ago

Sadly, I was a bit too optimistic with my previous post. I did fix a bug in my earlier code, but I've unfortunately reached the same conclusion, which is that it is only possible to override the tiny-slider behavior in a limited number of use cases. The issue is that there are too many cases where I can't overrule the tiny-slider logic due to the math used to calculate the necessary number of clones. There are 3 main issues:

1) When edge padding is present, tiny-slider created clones are used for the padded area at the start/end of the loop. These exposed sections will NOT have events. (Padded sections on center elements are fine, and when you actually navigate to the offending slides the Vue clones are used instead, so it's only the first and last padded sections that are affected.) Due to this line in the function that determines the clone count: return hasOption('edgePadding') ? result + 1 : result; I don't believe there's any way to avoid unwired clones when that option is present.

2) Similarly, it seems I can't create a scenario in which no clones are created in tiny-slider when either the autoWidth or fixedWidth properties are present. I can create the initial required amount of nodes in Vue, but since the tiny-slider interprets them as unique nodes, rather than clones, the logic that determines the clone count in those situations always results in (nodes- 1) * 2 clones.

3) The most common use case that doesn't work is when items > 1, where tiny-slider will create an additional items - 1 clones on either side of the slider for use in the final items - 1 transitions in either direction before rotating back into the properly cloned slides.

@viktorlarsson What this means is that, as far as I can tell, without making a change to tiny-slider to support custom clones or something similar, vue-tiny-slider can only fix uses cases where items === 1 and autoWidth and fixedWidth are both false (both of these generally result in items > 1). If you'd like I can still gate this behavior behind a prop flag that only works when the conditions allow for it, but given that it would need to come with documentation/disclaimers I'm not sure if it would be better to leave it broken consistently or to have it working in a small subset of cases. Sorry I couldn't be of more help!

stevenwr23 commented 4 years ago

I came across this issue recently as well. Could not fix it but found a workaround: I ended up adding the click event to the parent of the slider, and in the event handler check that the target was the intended button. Hope this helps until we can fix this.

hx-vasit commented 4 years ago

I came across this issue recently as well. Could not fix it but found a workaround: I ended up adding the click event to the parent of the slider, and in the event handler check that the target was the intended button. Hope this helps until we can fix this.

Can u give an example code?

stevenwr23 commented 4 years ago

Here is a simplified version of what I did.

This was my previous code (the one that does not work correctly)

<tiny-slider>
<div v-for="item in items">
<img v-bind:src="item.img">
<button v-on:click="openModal(item.url)">Open Modal</button>
</div>
</tiny-slider>

To fix it I added a click event to the parent of the slider instead of the button, and for the parameter, I used data- on the button:

<div v-on:click="handleClick"
<tiny-slider>
<div v-for="item in items">
<img v-bind:src="item.img">
<button v-bind:data-url="item.url">Open Modal</button>
</div>
</tiny-slider>
</div>

and the handleClick function:

function(event) {
if (event.target.dataset.url) {
this.openModal(event.target.dataset.url);
}
}

Hope this helps.

iSWORD commented 4 years ago

@stevenwr23 your solution worked beautifully, thank you so much for sharing it :)

I'm now stuck with passing properties to cloned elements though. Has anyone been able to workaround passing props?

MichelleZZZZ commented 3 years ago

loop: false, rewind: true,

This will do the same with the loop but without the clone issue.

TuDo73 commented 3 years ago

loop: false, rewind: true,

This will do the same with the loop but without the clone issue.

Thank you for this

BrunoCartier commented 2 years ago

Hello,

Any new information on this issue?

I faced it today: data bindings are not synced to Vue on cloned slides, so we loose the reactivity of the data.

(Using rewind: true just returns to the first slide and is not an acceptable option in my case, a real infinitive loop is asked by the designer)

brianecook commented 1 year ago

It's been a few years. Would be great to see a fix for this!