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

Center even number of horizontal slides when centerMode is true #2118

Merged
merged 3 commits into from
Sep 16, 2016

Conversation

engelfrost
Copy link
Contributor

Before: http://jsfiddle.net/o7nkhnow/2/ edit updated with correct use of rtl
After: http://jsfiddle.net/gy1jy44j/2/ edit updated with correct fix for rtl

This MR centers an even number of slides when _.slideCount <= _.options.slidesToShow for horizontal sliders.

If you want me to test more options please let me know and I'll be happy to do so.

@engelfrost
Copy link
Contributor Author

I just learned my rtl example was wrong. My fix does not work for rtl right now, but I'm working on a new commit right now.

The `rtl` condition was a mistake and completely unnecessary
@engelfrost
Copy link
Contributor Author

Fixed rtl. As noted in the fiddle, there are two issues I have not fixed:

  • Centering only works for horizontal sliders. Vertical sliders are top-aligned.
  • Centering is not fixed for slides with variable width where _.slideCount <= _.options.slidesToShow.

@leggomuhgreggo
Copy link
Collaborator

Nice! I made a demo confirming that asNavFor works.

@simeydotme
Copy link
Collaborator

wow... 👏 ... I've never seen another PR with this amount of tests :)
I'll take a look at them properly, and review the changed code soon :)

Thanks @engelfrost

@stha
Copy link

stha commented Feb 22, 2016

Any progress on that? 👍

@DJviolin
Copy link

DJviolin commented Mar 6, 2016

Is it possible somehow to align the screen's left the first slide, when centerMode and infinite set to true? And when you click the next button, it continue from the next slide which needs to be centered (slide 3 for example)?

@engelfrost
Copy link
Contributor Author

@DJviolin If I understand you correctly this PR does not do what you are looking for. There are examples above that demonstrate what this PR does and does not do.

@mowchan
Copy link

mowchan commented Apr 5, 2016

Are there any updates on this or when we may see a release?

Got the indentation wrong somehow, fixed it
@engelfrost
Copy link
Contributor Author

Any progress on getting this merged into master? Is there anything I can help with?

@leggomuhgreggo leggomuhgreggo self-assigned this Sep 16, 2016
@leggomuhgreggo leggomuhgreggo merged commit a737a4c into kenwheeler:master Sep 16, 2016
@leggomuhgreggo
Copy link
Collaborator

Thanks @engelfrost

@gsm-in
Copy link

gsm-in commented Dec 3, 2016

Thank you for this wonderful plugin and the update. But it seems that only the slick.js file has been updated and not slick.min.js.

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.

7 participants