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

If centerMode is true, no longer sets scrollToShow to 1. #659

Closed
wants to merge 1 commit into from

Conversation

eschow
Copy link

@eschow eschow commented Oct 15, 2014

Hoping to resolve #541, but would like to confirm that this doesn't affect any other settings.

@kenwheeler
Copy link
Owner

Have you tested this?

@eschow
Copy link
Author

eschow commented Oct 15, 2014

Yes, on the current project I'm developing, but I'm using only one set of
options.

On Wed, Oct 15, 2014 at 2:01 PM, Ken Wheeler notifications@github.com
wrote:

Have you tested this?


Reply to this email directly or view it on GitHub
#659 (comment).

@kenwheeler
Copy link
Owner

Thats hardly testing this. In order for pull requests to be entertained, they need to be tested ie8, with variable numbers of slides, slidesToShow, slidesToScroll, etc.

@eschow
Copy link
Author

eschow commented Oct 15, 2014

Noted. I'll run a test and then let you know.

On Wed, Oct 15, 2014 at 2:16 PM, Ken Wheeler notifications@github.com
wrote:

Thats hardly testing this. In order for pull requests to be entertained,
they need to be tested ie8, with variable numbers of slides, slidesToShow,
slidesToScroll, etc.


Reply to this email directly or view it on GitHub
#659 (comment).

@kenwheeler
Copy link
Owner

Please review Contributing.markdown and update this this PR.

@lupocreative
Copy link

Hi, did you manage to review this? centerMode:true is currently enforcing a fixed slidesToScroll of '1'. I would like to be able to use multiple options with different breakpoints if possible. Thanks.

@eschow
Copy link
Author

eschow commented Nov 20, 2014

Hi @lupocreative, I haven't had a chance to review this yet because I got sidetracked by other things. Let me take a look in the next 24 hours and get back to you.

@lupocreative
Copy link

Thank you @eschow! I have edited this directly in my slick.min.js file, and it seems to be working fine. I haven't tested it extensively, however.

@lupocreative
Copy link

(Oh, also I have tried it out with multiple breakpoints and it seems to work fine.)

    $('ul.portfolioSubpages').slick({
        slide: 'li',
        centerMode: true,
        centerPadding: '47px',
        slidesToShow: 3,
        slidesToScroll: 3,
        prevArrow: '<div id="slickPrev"><img src="/2014/img/arrowLeft.png" alt="Previous" width="32" height="32" /></div>',
        nextArrow: '<div id="slickNext"><img src="/2014/img/arrowRight.png" alt="Next" width="32" height="32" /></div>',
        responsive: [
            {
              breakpoint: 480,
              settings: {
                prevArrow: '<div id="slickPrev"><img src="/2014/img/arrowLeft.png" alt="Previous" width="32" height="32" /></div>',
                nextArrow: '<div id="slickNext"><img src="/2014/img/arrowRight.png" alt="Next" width="32" height="32" /></div>',
                centerMode: false,
                centerPadding: '40px',
                slidesToShow: 1,
                slidesToScroll: 1
              }
            }
        ],
        onInit: function(){
            $('ul.portfolioSubpages').show();
        }
    });

@eschow
Copy link
Author

eschow commented Nov 22, 2014

I'm so sorry, @lupocreative. I have a project deadline at work this weekend and haven't been able to revisit this. I'll let you know when I get a chance.

@lupocreative
Copy link

No worries at all, @eschow. I appreciate your input. It appears to be working fine with the edits above, so there's no urgency from my perspective. We're going to stick with the edits we've already made but I'll keep an ear to the ground both here and on @kenwheeler's releases for any future work on this issue.

@eschow
Copy link
Author

eschow commented Dec 26, 2014

OK, @kenwheeler. I've been dragging my feet and got tied up with other work projects but finally managed to test this on IE8 - 11, Chrome 39.0., FF33.1 and Safari 7.1.2 with various options.

Let me know if there's anything else I can do.

@kenwheeler
Copy link
Owner

Ok @eschow I'll review this and see if I can get it in for 1.4

@befreestudios
Copy link

@eschow, @kenwheeler... this fix only works if there are an even number of slides.

For example try this fix with 6 slides (works), and then try with 7 slides (broken). I'm trying to track down where this is failing, but in the meantime if someone else can figure it out IT WOULD HELP IMMENSELY. THANKS!!!!

@eschow
Copy link
Author

eschow commented Jan 15, 2015

From my understanding, centerMode only works with an odd number of slides because there's nothing to center in an even set of slides. Correct me if I'm wrong, @kenwheeler. That was just the assumption I made when I was using this library.

@befreestudios
Copy link

That seems very inconsistent and restrictive. When working with a variable amount of data this would be unacceptable. If centerMode is to work with slidesToMove it should work with both odd and even number of slides. You would have to assume that infinite was true, but if you scrolled (by 3) past the number of slides available, then it should go back to the beginning. But if infinite was false, then you could just stop at the end. So for example if number of slides is 7 and current slide is 5, when you navigate the centered slide would be slide 1(like a cyclical list, but moving by 3) ~ Thoughts?

@kenwheeler
Copy link
Owner

Center mode works with any number

@befreestudios
Copy link

Thats not what I'm seeing (when centerMode == true), unless I'm missing something.

With @eschow fix, slidesToScroll = odd, and an even number of slides, it breaks. And without the fix, no matter what number is set for slidesToScroll, it only jumps one. Try applying the proposed fix above, with slidesToScroll && slidesToShow = 3, and different numbers of slides. It seems to break when numberOfSlides is not divisible by slidesToScroll or something.

@kenwheeler
Copy link
Owner

I'm saying in general, not this fix

@befreestudios
Copy link

Thanks again for the help and quick responses. True, in general. Although, when centerMode == true && slidesToScroll == 3, this does not work. Is that a valid bug that has been logged? If not, could we consider this a bug?

To that point, I know this fix above is an attempt to fix this, however all I'm pointing out is that this fix does not fix it for -n number of slides, which is what I would expect. I'll still be digging in too and see If I can find a solution, but please keep me informed as well....

@befreestudios
Copy link

@eschow BTW the title of this issue is totally misleading. There is no property "scrollToShow", it's either slidesToShow, or slidesToScroll. In your fix you remove _.options.slidesToScroll = 1; and then set the slidesToShow so I'm completely unsure as to what you are trying to accomplish.

@kenwheeler To be clear, The issue I'm having is when centerMode == true && slidesToScroll == 3, and the number of slides is not divisible by slidesToScroll (For Example: numSlides == 7 && slidesToScroll == 3)

@eschow
Copy link
Author

eschow commented Jan 15, 2015

My apologies. Multitasking is not my strong suit and I filed it while working on something else.

On Jan 15, 2015, at 12:55 PM, Burton Podczerwinski notifications@github.com wrote:

@eschow BTW the title of this issue is totally misleading. There is no property "scrollToShow", it's either slidesToShow, or slidesToScroll. In your fix you remove _.options.slidesToScroll = 1; and then set the slidesToShow so I'm completely sure what you are trying to accomplish.

@kenwheeler To be clear, The issue I'm having is when centerMode == true && slidesToScroll == 3.


Reply to this email directly or view it on GitHub.

@befreestudios
Copy link

@eschow No worries, just wanted to point that out. :)

@kenwheeler This will partially fix the issue I'm seeing. If in centerMode, if the slideCount is divisible by the slidesToScroll property, allow it. Otherwise, set slidesToScroll = 1. However, the case where slideCount % slidesToScroll !== 0, is still not accounted for. <- which is what is breaking for me.

if (_.options.centerMode === true) {
            if (_.slideCount % _.options.slidesToScroll !== 0) {
                _.options.slidesToScroll = 1;
            }
        }

@kenwheeler
Copy link
Owner

ok @befreestudios I'm gonna review this and try and get it in the next release. Been busy with the kid and the holidays, just getting back into the swing of things.

@befreestudios
Copy link

@kenwheeler, awesome. Thank you. I completely understand (have three three kids as well). Thanks again and let me know if you need anything else from me. Thx.

@mhulse
Copy link

mhulse commented Apr 1, 2015

👍 on this fix.

When centerMode is enabled it appears as though slidesToScroll gets fixed to 1.

@simeydotme
Copy link
Collaborator

This is a bit of a mess... and very old... I'll close it and try to note down this issue for the upcoming version 2.

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

Successfully merging this pull request may close these issues.

Update documentation on slidesToScroll
6 participants