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

Add pagination feature #327

Merged
merged 15 commits into from Jun 20, 2019
Merged

Conversation

omares
Copy link
Contributor

@omares omares commented Mar 13, 2019

Hey @jedrzejchalubek, i added the page navigation features as requested in #252

The syntax was slightly changed from the suggestion.

|> pages to the right
|< pages to the left

Additionally i added the waitForTransition flag which allows developers to decide if they want to create an artificial input delay or not.

Let me know what you think, ill gladly update the documentation when this features passes your requirements.

Todos

  • Fix pagination in combination with carousel

@garygreen
Copy link

Thanks so much for this Ota, great work! 👍

@jedrzejchalubek
Copy link
Member

Wow, great 💯. Please, give me a few days to review it :)

@garygreen
Copy link

garygreen commented Mar 13, 2019

It's very elegantly coded, my only suggestion is maybe having this as more of a separate module which exposes the page number, total number of pages, etc.

Reason being is I believe this could be very useful in #241 #251 - if this was a component that you can interact .getPage() .getTotalPages() blah blah, it could then be used with the navigation/dots. At the moment this PR hard codes the calculations inside the move function. It could also then be optional so could be code-split if folks don't need pagination based controls/navigation.

But in any case this PR is already 💯

@omares
Copy link
Contributor Author

omares commented Mar 13, 2019

Thanks for the quick response, really appreciated :)
I am currently adapting the overscroll feature of the pagination as it currently does not feel right when bound is deactivated.

@garygreen I agree with your ideas. My suggestion would be to tackle that in a different PR so that we can move one quickly with this one.

@jedrzejchalubek For the sake of testing i created a few demo implementations. Do you want me to commit these too? I created a gist in case you would like to take a preview.

The styles are copied from the official https://glidejs.com website 🙈

/edit

After toying around with my demo i found a few inconsistency that i fixed. Unfortunately the code becomes a bit more complex as bound is messing up the length index.

Besides that i have to take a look at the carousel mode and pagination. Right now i do not understand how to prevent the carousel from rewinding when exceeding the last element.

@omares
Copy link
Contributor Author

omares commented Mar 14, 2019

@jedrzejchalubek please wait with your review, after investigating the carousel issue i found out that further changes in the code are required and other components, like Translate, have to be adapted too

/edit

@jedrzejchalubek could you please quickly tell me if the "countableSteps" feature in form of >$int and <$int is still something that should be supported? It is not showing up in the docs. If not i would also remove that.

@jedrzejchalubek
Copy link
Member

@omares Ok, let me know :)

countableSteps are used by Swipe component to move more than one slide if we have a long swiping (https://github.com/glidejs/glide/blob/master/src/components/swipe.js#L141). For now, It should be still supported.

@omares omares force-pushed the add_pagination branch 2 times, most recently from 5db80b1 to a3a6afc Compare March 19, 2019 00:00
@omares
Copy link
Contributor Author

omares commented Mar 19, 2019

That took longer than expected :)

  • Refactored the translate code.
    • Fixed issue with gap calculation, you don't have to add random gap values anymore to receive the correct translate distance.
    • Unified translate code. Moving left or right does not require individual calculations.
  • Refactored Run.calculate
    • <, >, $steps<, >$steps, |< and |> now share the same code to move around, making the code easier to understand. This should also allow to easily add other navigation concepts.
    • Added support for carousel pagination.

@omares
Copy link
Contributor Author

omares commented Mar 19, 2019

@jedrzejchalubek Ready to review :)

@omares
Copy link
Contributor Author

omares commented Mar 26, 2019

Hey @jedrzejchalubek is there anything i can do to make the review easier to you?

@omares
Copy link
Contributor Author

omares commented Apr 12, 2019

@jedrzejchalubek not trying to be a pain in the ass, but any news here?

I would totally love to get this merged so i can remove this from my todo list :D Also this kinda prevents me from contributing more.

@jedrzejchalubek
Copy link
Member

jedrzejchalubek commented Apr 17, 2019

@omares I was a little busy over the past few weeks. Sorry about that :)

I hope I can make an appropriate review that this PR deserves the following weekend. One more time - great work!

Hey @jedrzejchalubek is there anything i can do to make the review easier to you?

The test are in place. That's all i wish for :)

@jedrzejchalubek
Copy link
Member

jedrzejchalubek commented Apr 20, 2019

@omares I have updated your PR with additional tests so I will be sure running behaves in the same manner as before. The Run component changes a lot, so I had to do that. Also, updated this PR with the latest changes on the master branch (some small bug fixes). Forgive me for these direct commits :)

I found a few edge cases that need to be addressed and behaviors that I want to behave differently. I will prepare and push you falling tests that you would need to get to the green. Are you ok with that workflow?

When we get this done, we will take care of refactorization to achieve better compression :)

@jedrzejchalubek
Copy link
Member

Added regression test :) Here what I want to achieve:

  • I would like that new pattern was moving the slider between viewports rather than a number of slides defined in perView
  • We have to double the number of clones, because in carousel mode and rewind turned on, so we avoid seeing an empty space when looping and jumping translation

@omares
Copy link
Contributor Author

omares commented Apr 26, 2019

Hey @jedrzejchalubek, awesome and thanks for taking care. I will look at the test!
Right now i am swamped :( hoping it wont take me to long.

@jedrzejchalubek jedrzejchalubek added this to the 3.4.0 milestone Apr 26, 2019
@omares
Copy link
Contributor Author

omares commented May 10, 2019

Hey @jedrzejchalubek do you mean viewport as in "number of slides" currently visible?

Edit: Never mind, figured it out and fixed it ;) Now onto the cloning behaviour.

@omares
Copy link
Contributor Author

omares commented May 10, 2019

@jedrzejchalubek i fixed both issues.

Regarding the cloning behaviour i went a slightly different way. As it is about avoiding the emptiness i solved it by adding the required number of clones to prevent the space, instead of doubling it. As less is more for performance :)

@jedrzejchalubek
Copy link
Member

jedrzejchalubek commented May 13, 2019

Great! Going to give this a second review and we are ready to merge.

I'll take care of small refactoring later on :)

@EvilLooney
Copy link

@omares

I am testing out your commit and have to say it's working pretty well.

My only comment would be that it would be nice if the paging could be applied to "dragging" as well.

Great work!

@omares
Copy link
Contributor Author

omares commented May 29, 2019

thanks @EvilLooney for the feedback. i really hope that @jedrzejchalubek finds time soon to finish this as the pr is already a few weeks old.

@marcosmyara
Copy link

@omares does this update mean once the slider is loaded (with no rewind option and focused on slide 0) there will be a glide__arrow--disabled class added to the left arrow button? and also the class will be added to the right arrow button once the slider reaches the end? Cause that is really what i am expecting, i hope i got it right from what i read here. thanks!

@omares
Copy link
Contributor Author

omares commented Jun 3, 2019

@marcosmyara setting the appropriate disabled class is implemented in my second PR #328 which unfortunately is also pending.

@vincent-lu
Copy link

Hey guys any ETA on this one?

Round perView incrementer so there is a proper number of clones when we have an odd number of slides
Add tests for testing clones generation for `even` and `odd` number of `perView` option
@jedrzejchalubek jedrzejchalubek merged commit b32ea37 into glidejs:master Jun 20, 2019
@jedrzejchalubek
Copy link
Member

I've made the final review and PR has been merged. 🎉🎉🎉

Thank you for your contribution @omares. Fantastic work :) v3.4 that includes this feature should be released within this week.

@rockymountainhigh1943
Copy link

@jedrzejchalubek — is there a read on when this feature will be released?

@GiancarlosIO
Copy link

👀

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

Successfully merging this pull request may close these issues.

None yet

8 participants