-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
Added outer edge clipping #2635
base: master
Are you sure you want to change the base?
Conversation
Outer edge clipping only affects slick with variable width on. This new option `outerEdgeLimit` allows the last element of the slider to stop at its limit, instead of leaving empty space.
Thanks! |
I noticed a bug. When more than one item is visible and the outer edge has been reached, the next button should be disabled, but it stays visible. Unfortunately I currently don't have time to fix it up. If someone would take a look at that it would be great. |
Brings in Slick PR on most recent master branch code: kenwheeler#2635
Port in Slick PR kenwheeler#2635
Hi I have tried to solve the same issue using a different approach unfortunately I didn't check other pull requests before starting to write. It seems your solution is more efficient, does it work fine? |
@falkartis I haven't been able to test it a lot and still haven't fixed the bug I wrote about before. From what I remember from the code, to fix the bug I need some small conditions here and there (mainly to enable and disable prev-next buttons), but never got around to do them. |
Well, the prev-next buttons, my approach has the same issue. I'll write a
fix if my costumer asks me to.
|
@RicardoEPRodrigues Exactly what I need. Didn't found the fix for the next button yet. But I'll use this in my website. Thanks ! |
Thank you! You are a life saver. |
I didn't test or check if this was the solution, and even if you already tried this, but, what if we call |
@RicardoEPRodrigues after a quick test, i saw that this example fiddle with release didn't update the right arrow as well, so, maybe is not a problem with this patch itself. But indeed fix the problem that we have with the end of the slide :D 🌮 🎉 |
…hen infinite = false: When infinite is set to false the last slide would come all the way to the left, thus leaving a whitespace the size of the slider container. This use [this pull request](kenwheeler/slick#2635) on source library to correct this behavior.
Anyone found a solution to disable the next button when the outer edge has been reached ? |
Unfortunately I didn't get that far. I do suspect there may be a function, that determines if the buttons should be shown, that is being called too early. |
@kenwheeler @leggomuhgreggo i'm using this lifesaving fix for about one year now in several sliders. Will this ever be finished and integrated into the base?! |
I started this for a project I was working on and decided to share it. Since I didn't had the time to fix it, I hoped someone would. Glad I could help you out! |
@kenwheeler @leggomuhgreggo is this going to be merged in base at some point? |
Fixed the problem with the arrows for me:Added new condition to the updateArrows function: Slick.prototype.updateArrows = function() {
var _ = this,
centerOffset;
centerOffset = Math.floor(_.options.slidesToShow / 2);
if ( _.options.arrows === true &&
_.slideCount > _.options.slidesToShow &&
!_.options.infinite ) {
_.$prevArrow.removeClass('slick-disabled').attr('aria-disabled', 'false');
_.$nextArrow.removeClass('slick-disabled').attr('aria-disabled', 'false');
if (_.currentSlide === 0) {
_.$prevArrow.addClass('slick-disabled').attr('aria-disabled', 'true');
_.$nextArrow.removeClass('slick-disabled').attr('aria-disabled', 'false');
} else if (_.currentSlide >= _.slideCount - _.options.slidesToShow && _.options.centerMode === false) {
_.$nextArrow.addClass('slick-disabled').attr('aria-disabled', 'true');
_.$prevArrow.removeClass('slick-disabled').attr('aria-disabled', 'false');
} else if (_.currentSlide >= _.slideCount - 1 && _.options.centerMode === true) {
_.$nextArrow.addClass('slick-disabled').attr('aria-disabled', 'true');
_.$prevArrow.removeClass('slick-disabled').attr('aria-disabled', 'false');
}
}
if ( _.options.arrows === true &&
_.slideCount > _.options.slidesToShow &&
!_.options.infinite && _.options.outerEdgeLimit){
var lastSlide = _.$slides.last();
var lastSlideOffsetRight = ($(window).width() - (lastSlide.offset().left + lastSlide.outerWidth()));
var sliderOffsetRight = ($(window).width() - (_.$slider.offset().left + _.$slider.outerWidth()));
var lastSlideOffsetSlider = lastSlideOffsetRight - sliderOffsetRight;
if(lastSlideOffsetSlider > -1) {
_.$nextArrow.addClass('slick-disabled').attr('aria-disabled', 'true');
}
else {
_.$nextArrow.removeClass('slick-disabled').attr('aria-disabled', 'true');
}
}
}; Added updateArrows to the postSlide function: Slick.prototype.postSlide = function(index) {
var _ = this;
if( !_.unslicked ) {
_.$slider.trigger('afterChange', [_, index]);
_.updateArrows();
_.animating = false;
_.setPosition();
_.swipeLeft = null;
if ( _.options.autoplay ) {
_.autoPlay();
}
if (_.options.accessibility === true) {
_.initADA();
}
}
}; Now you are able to apply styles to .slick-disabled or :not(.slick-disabled) |
When I have time I'll update my branch. |
Edited my post: |
Hey folks, I'm having this issue too and it would be really nice if this PR is merged to the library. Is there anything blocking it? Is it tests? Looking to help so we can have this available for everyone. Thanks! |
@joaopslins I don't know why it still hasn't been merged. I created this fix and @sanchez089 contributed to "polish the edge", but still no comment or merge from the developers. |
@RicardoEPRodrigues I see, I've copy pasted our code here to check if it solves my issue and it does! I just noticed 1 minor issue:
EDIT: My slick call is as follows:
|
Just found another issue, it looks like there is a certain threshold where the last element does not fully appear, the |
@RicardoEPRodrigues Got the example for the threshold not working on the last element: http://jsfiddle.net/yfvp78xr/19/ The last item does not show fully, the button appears but does not work. Probably your screen resolution can affect this repro but I got this on 1920x1080 and 1366x768 resolutions. About the arrows not appearing when opening the page, I couldn't reproduce on the fiddle. By debugging, I can see that when the Because of that, the last arrow is set as disabled. The |
Managed to solve the hidden arrow issue by calling EDIT: Found the culprit. I was setting a |
@RicardoEPRodrigues about the threshold bug I've posted here earlier, I noticed it works as expected if I comment this on line 1232: // [...]
var slideOuterWidth, slideOffset, slideRightBoundary;
slideOuterWidth = $(slide).outerWidth();
slideOffset = slide.offsetLeft;
/* if (_.options.centerMode !== true) {
slideOffset += (slideOuterWidth / 2);
} */
slideRightBoundary = slideOffset + (slideOuterWidth);
// [...] Could you explain what was the motivation of this condition? For me it only works if I remove this piece of code. |
Removed unnecessary addition to the offset variable, that made the last element inaccessible. Fixed UpdateArrows where last element width exceeds the window width, now works correctly when last element is reached.
I took some time a fixed 2 or 3 bugs. Basically I restricted 👏 👏 Next Bug I removed the code referenced by @joaopslins #2635 (comment), thank you! 🎉 👏 👏 Next Bug I also fixed an annoying bug when the last element has width greater than the window, this would allow the arrows to remain active, which is now fixed! 😄 Try the fixes here: http://jsfiddle.net/RicardoEPRodrigues/284ruc1d/ . Mess around with the widths to test it out. |
Nice! Thanks for these changes! The center mode really doesn't make much sense with this new option on so the restriction is good. Just a heads up, I think the jsfiddle you posted didn't include the latest changes, I've pasted the new code on the previous fiddle. Here is an updated version: http://jsfiddle.net/bcgLaj46/ |
Hey, I just wanted to comment on the lack of updates in the Those doing Pull Requests are the ones keeping this alive. Thank you everyone! |
Hello again folks, Got another issue when I used a
I managed to get this working by adding a new condition on the method if (_.options.outerEdgeLimit && _.options.centerMode === false) {
outerEdgeSlideNumber = _.getOuterEdgeSlideNumber();
if (outerEdgeSlideNumber < index) {
index = outerEdgeSlideNumber
}
if (index < 0) { // new condition
index = 0;
}
} Fiddle with this fix: http://jsfiddle.net/j3dt5suv/3/ I'm not sure if more checks are needed elsewhere but this worked for me, what do you think @RicardoEPRodrigues ? |
@joaopslins Hello! On future improvements I invite you to do pull requests on my branch, this way we can workout the kinks more easily 😄 Ok, some notes here:
|
It turns out that the index bug is from Slick, not from the New fiddle available: https://jsfiddle.net/RicardoEPRodrigues/284ruc1d/ Please comment and report more bugs. |
I'm trying to use this update in one of my sites but it seems to be breaking my slider. The fiddle you provided (https://jsfiddle.net/RicardoEPRodrigues/284ruc1d/) also doesn't seem to be working? Maybe I'm missing something here? |
For some reason it looks like jsfiddle is broken. Not even the default jsfiddle works (this one). @k-cogswell I'm using this PR code and it is working well. It will be hard to help you without the jsfiddle working though. |
I am looking for this PR to be merged. Please let me know if this can be merged ? |
How do you do this without changing the slick.js? Please let me know |
The PR is not yet merged by the SlickJS team yet. We cant accomplish this
without changing the slickjs library code.
Thanks,
Sindu
…On Thu, Dec 19, 2019 at 12:00 PM shielakn ***@***.***> wrote:
How do you do this without changing the slick.js? Please let me know
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#2635?email_source=notifications&email_token=AEYP3YS54M6USVVJHSTAIWLQZOZFRA5CNFSM4CXX4OWKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHKNTFA#issuecomment-567597460>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEYP3YV7BUNS6FTMYJNAFZDQZOZFRANCNFSM4CXX4OWA>
.
|
Unfortunately this fix doesn't work properly. When you use mobile slider and drag to the end, several times, then you won't be able to drag back. My solution was to count how many items can fit in the visible area, and put that number in |
Hey, @tylik1 can you show us the changes in a jsfiddle? So we can try them out and correct the code. If you'd like you can make a pull request with the changes on my repository! 😄 |
Hi, any update? |
I've implemented this, more like a hack. I'm calculating the width of the slider container, then counting number of slides, that fits that container, to pouplate
|
The |
I have added Version: 1.9.0 in my project it is working good White spacing issues solved and it is working properly but I have seen the new issue is that when I use the variableWidth: true, slider next arrow not disable on last slide. |
$('.items').slick({
infinite: false,
slidesToShow: 1,
variableWidth: true,
centerMode: true,
centerPadding: '0',
outerEdgeLimit: true,
}); OR $('.items').slick({
infinite: false,
slidesToShow: 1,
variableWidth: true,
outerEdgeLimit: true,
}); |
EDIT: The issue was caused by |
Outer edge clipping only affects slick with variable width on.
This new option
outerEdgeLimit
allows the last element of the slider to stop at its limit, instead of leaving empty space.