Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

[Pagination] Add first/last link & constant congif options. #173

Closed
wants to merge 1 commit into from

Conversation

bekos
Copy link
Contributor

@bekos bekos commented Feb 26, 2013

  • Options for text of first, previous, next & last links.
  • Option "boundary-links" whether to display "first" & "last" link
    (default: false)
  • Option "direction-links" whether to display "previous" & "next" link
    (default: true)
  • Added tests!!!

@bekos
Copy link
Contributor Author

bekos commented Feb 26, 2013

After the #159 problems, i believe i got a better idea to sort this out.
(As we are supposed to do) Moved the logic of whether or not to display extra buttons into the linking function instead of the template.

With this approach, i could make pagination more configurable.
So, now we can display pagination as
[First] [Previous] [1] .... [10] [Next] [Last]
[Previous] [1] .... [10] [Next]
[First][1] .... [10] [Last] or just
[1].... [10]

Also there is no hiding of elements, so no CSS problems :-)

I hope you like it!

@pkozlowski-opensource
Copy link
Member

@bekos This version looks much better!

@petebacondarwin This lgtm but it would be great if you could have a quick look

// Add page number links
for(var i = 0, n = Math.min(maxSize, scope.numPages); i < n; i++) {
var number = startPage + i;
var page = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You repeat this var page = { number: .., text: .., active: .., disabled: .. } a lot. Couldn't you create a function makePage(number, text, active, disabled) {} function and use that? It would reduce your lines of code a lot and you'd avoid repeating yourself 👍

@ajoslin
Copy link
Contributor

ajoslin commented Feb 28, 2013

All looks good, except the one comment I made. Thanks @bekos! This needed a bit of a refactor.

@bekos
Copy link
Contributor Author

bekos commented Mar 1, 2013

@ajoslin Fixed! Also i fixed the iteration over page numbers.

@pkozlowski-opensource
Copy link
Member

@bekos this looks far better now!

Could you squash all the commits into a single one and rebase it on top of the current master? This would help me a lot in merging this PR.

 * Options for text of first, previous, next & last links.
 * Option "boundary-links" whether to display "first" & "last" link
   (default: false)
 * Option "direction-links" whether to display "previous" & "next" link
   (default: true)
 * Added tests!!!
 * Fix demo to show all possible configuration options
@bekos
Copy link
Contributor Author

bekos commented Mar 2, 2013

@pkozlowski-opensource I think it's OK now.

@pkozlowski-opensource
Copy link
Member

@bekos thnx a lot! I've landed it as 0ff0454.

You are quite active on the project, this is great! We all count on more cool contributions from you!

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

Successfully merging this pull request may close these issues.

3 participants