slick
slick copied to clipboard
kenwheeler
setSlidesClasses selector incorrect when index is 0
Mainly to fix #3271 where users are complaining about center mode jumping when going from last to first slide, I think there are other tickets where this may be the cause of jerky animations
This js fiddle shows the issue http://jsfiddle.net/paulbarrett79/5z11Lo7v/2/ This fix ensures the correct clone gets the slick-center class when going from last to first in center, infinite mode.
My fix is shown working here https://codepen.io/PaulBarrett79/pen/WdWYaO
Thanks for this. It looks as though a separate fix is still needed for slick-active with infinite scroll, but once I enabled centerMode and started targeting slick-center instead this worked a treat.
Same issue here - thanks for creating a fix @PaulBarrett79.
You can make a slight optimisation in the index calculation, as some of the variables effectively cancel each other out. Change from,
allSlides
.eq(0)
.eq(allSlides.length - 1 - _.options.slidesToShow - (_.slideCount - 1 - _.options.slidesToShow))
.addClass('slick-center');
to,
allSlides
.eq(0)
.eq(allSlides.length - _.slideCount)
.addClass('slick-center');
@domstubbs
I'd suggest that the .slick-active class remains as it is. In my opinion, it should only be added to the currently active slide(s) and a cloned slide should never be active in that sense.
It also provides a different way of targeting an element or set of elements, rather than having 2 classes providing the same functionality.
@IAmAdamTaylor nice, happy for this to be what the pull request should be changed to (by the powers that be). Still a bit new to all this! Glad it worked for you.
Thank you, it helped a lot, just wondering why it's still not in the last version of slick.
@PaulBarrett79, THANK YOU A LOT )) I f***ed with that almost two whole days ))
@PaulBarrett79, My question may seem stupid, but could you tell me what exactly should I add to jsfiddle example? There is to many code in codepen example :D
@Macieyou Hi! In the codepen I had to add the whole of the slick library into the code because I couldn't import it another way that included my change quickly! The change I made is the one in this pull request.
@kenwheeler we would really appreciate if this fix could make its way to the next release :wink:
@kenwheeler also here too - use this mode a lot more often these days, would be great if it was merged
Well down! But I have one question, why the demo work well... @PaulBarrett79
Well down! But I have one question, why the demo work well... @PaulBarrett79
It's a great question and one that I don't have the answer to. However, lots of people have had the same issue and this seems to fix it for them too.
Please merge this branch and put out a new slick-carousel version. I really need the fix :)
Did not help me Help me figure out why when it comes to switching from the last to the first slide and vice versa there is a jump of items. There should be the same transition as between the first and second slides http://jsfiddle.net/drtvader/of2bjudr/24/
Perfect fix for zooming the centered slide. I couldn't for the life of me figure out why this was an issue, when even the official site has a working example of a centered carousel with the active slide zoomed.
+1 for merging this, such a simple fix that improves the plugin immensely.
@PaulBarrett79 - Thank you!! This issue has just wasted half my day...if only I found your solution sooner :)
+1 for merging this too
Hello. Why in slick 1.9 does not work when you move forward? https://codepen.io/anon/pen/wQjZmp
Great solution! But if I have 'slidesToShow: 5' and 'focusOnSelect: true', then I click on the -2 indexed slide (I mean the one on the far left), the fix does not work. This is the example (forked from @PaulBarrett79 ) https://codepen.io/giacomocarra/pen/xMxJEX
Any idea? Thank you
@giacomocarra
I've added a fix for your issue in my forked CodePen. See https://codepen.io/IAmAdamTaylor/pen/exmrdy and search for PR#3309 BEGIN in the JavaScript to find the changes.
By detecting the cloned slide index that we are moving to, we can apply the slick-center class directly. The previous fix was only applied if moving to the first or last slides.
I've tested with different numbers of slidesToShow and different amounts of slides and it seems to handle them all. If you find a bug, let me know.
Quick summary of changes (I might create a forked repo/pull request with this inside at some point and reference it back here):
In @PaulBarrett79 's version of Slick, find lines 2324 - 2356:
if (_.options.infinite === true) {
if (index >= centerOffset && index <= (_.slideCount - 1) - centerOffset) {
_.$slides
.slice(index - centerOffset + evenCoef, index + centerOffset + 1)
.addClass('slick-active')
.attr('aria-hidden', 'false');
} else {
indexOffset = _.options.slidesToShow + index;
allSlides
.slice(indexOffset - centerOffset + 1 + evenCoef, indexOffset + centerOffset + 2)
.addClass('slick-active')
.attr('aria-hidden', 'false');
}
if (index === 0) {
allSlides
.eq(allSlides.length - _.slideCount)
.addClass('slick-center');
} else if (index === _.slideCount - 1) {
allSlides
.eq(_.options.slidesToShow)
.addClass('slick-center');
}
}
Replace those lines with:
if (_.options.infinite === true) {
if (index >= centerOffset && index <= (_.slideCount - 1) - centerOffset) {
_.$slides
.slice(index - centerOffset + evenCoef, index + centerOffset + 1)
.addClass('slick-active')
.attr('aria-hidden', 'false');
} else {
indexOffset = _.options.slidesToShow + index;
allSlides
.slice(indexOffset - centerOffset + 1 + evenCoef, indexOffset + centerOffset + 2)
.addClass('slick-active')
.attr('aria-hidden', 'false');
var cloneIndex = indexOffset + 1 + evenCoef + _.slideCount;
if(index > centerOffset) {
cloneIndex = indexOffset + 1 + evenCoef - _.slideCount;
}
allSlides
.eq(cloneIndex)
.addClass('slick-center')
}
}
Well down! But I have one question, why the demo work well... @PaulBarrett79
Interestingly, if you take a look at the affected lines of the Slick library loaded on the demo site, it is actually running a version which includes this fix (or a variant of, it uses slightly different variables)!
So it seems the creator knew about this and fixed it specifically for their demo.
Found a bug in the version I posted above. It wouldn't work when slidesToShow is set to 1.
I've since created a fork of the repo as mentioned and fixed the bug. See commit details here https://github.com/IAmAdamTaylor/slick/commit/2986a0dc4cc79db67f58acf654280e5b5e5e1f74 It also has some small optimisations over the previous version posted.
The code inside the CodePen (https://codepen.io/IAmAdamTaylor/pen/exmrdy) is also updated to match.
Thank you very much @IAmAdamTaylor , the fix works! Also, I implemented it in the last version of Slick (1.9.0) and works fine. It can be found here in my forked pen if someone needs. https://codepen.io/giacomocarra/pen/xMxJEX