feat(pagination): Add force-ellipses and boundary-link-numbers options

[OzzieOrca]

Setting force-ellipses to true: When we have more pages than can be shown and rotate is true, ellipses will be shown (without it they are only shown when rotate is false. Example:
Previous, ..., 4, 5, 6, ..., Next

Setting boundary-link-numbers to true: When we have more pages than can be shown, the first and last page numbers will always be shown. Example:
Previous, 1, ..., 4, 5, 6, ..., 9, Next

Cleans up and builds off of #3564 which only had force-ellipses. (PR now closed)
Based off #3035 and makes ellipses for rotating an opt-in feature
Addresses issue #2924 (wanting ellipses when rotating)
Addresses issue #3064 (wanting first and last page numbers)

[wesleycho]

The main concern I have with this as a feature is that this increases the complexity of the pagination exponentially, since now we have a link between the start and end that has to be maintained in addition to the features, which should have been split into two PRs.

In addition, I see a commit by @wtc-dstone here - it is bad form to open a PR with someone else's commits unless the person gave explicit approval, for license reasons.

[OzzieOrca]

Ok. Where do you see the complexity being the issue? In the added options (and documentation) that the user sees or the underlying code or both?

What would you like to see? You didn't really give any action points.

Also I don't know much about licensing issues. I was trying not to just steal the code he wrote without giving him credit. What should I have done/how should I fix that?

If I were to clean it up I think I would just keep the boundary-link-numbers option and possibly think about how to name it better. Having the first and last page in the pagination is a common feature on a lot of sites (and the feature I wanted) and is useful for seeing how many pages there are. Do you think I should drop the force-ellipses option? I was trying to add a feature someone else had requested and these 2 options seemed to share a lot of code. It has been a while and I would have to look at it again to remember how much code they still share.

Let me know how you would like to see this move forward. I'd be willing to work on it more. Thanks for the feedback.

[wesleycho]

Re: Licensing - I understand the intent is good, but if the person didn't file the PR, there is always the chance that the person does not wish to give permission for the code to get merged back into the main project. That person owns the code he/she wrote, so the person would have every right.

The complexity lies in changing the data structure at heart to not be just an array, but a doubly linked list that loops at the ends, like a carousel. Given the complexity that results and given that this is a far departure from a normal pagination control UXwise, the rotate feature should not be present in the control in this project in my opinion.

As a consequence, the action items should be to

1. Remove the bad commit in question
2. Remove the rotate feature from the PR

In the future, please do separate PRs for separate features - it makes it much easier to revert painlessly if it is decided that a change needs to be reverted.

[OzzieOrca]

Hey I think I'm a little confused. I'm struggling trying to explain it concisely. Hopefully some of this makes sense and you are able to clear it up. Maybe in my confusion about this, I'm proving the complexity of this feature haha. I didn't think it would take this much thought. Here are my two main thoughts at trying to explain my confusion:

What does "remove the rotate feature" mean?

Maybe we have different ideas of what "rotate" means and what removing it looks like.

If you want to see all the options that are available based on where the PR currently is at check out: http://plnkr.co/edit/G7phcuFXSgY1vRPnMIo8?p=preview

Rotate could mean:

1. The preexisting rotate pagination setting which when false makes it seem like it is "rotating" in groups. From docs: rotate (Defaults: true) : Whether to keep current page in the middle of the visible ones.
2. The new force-ellipses option that makes it seem like it is "rotating", keeping the current page in the center.
3. The concept to keep current page in the middle (similar to above) (I don't think that the word rotate describes this well but that's what the option is)
4. The concept where clicking on an ellipsis shifts all current numbers aside and brings up the next few selecting the first in that section. Example clicking the right ... in ... 3 4 *5* 6 7 .. changes it to ... *8* 9 10 11 12 ... (asterisks indicates currently selected)
5. The concept where clicking on an ellipsis shifts some current numbers aside and moves what would have been at the ellipse's position to the center. Example clicking the right ... in ... 3 4 *5* 6 7 .. changes it to ... 6 7 *8* 9 10 ...
6. The concept where clicking on an ellipsis shifts all current numbers aside and brings up the next few, moving the selected page to the center of the next section. Example clicking the right ... in ... 3 4 *5* 6 7 .. changes it to ... 9 10 *11* 12 13 ...

If the PR was merged as is it would have options to enable the functionality of 4 and 5. Before it only had 4. Idk if 6 would ever be wanted.

What does removing the "doubly linked list that loops at the ends" and its complexity look like?

I don't think I added any new data structures.

The only code this PR modified was the // Add links to move between page sets section of the getPages() function (besides default config, tests, and documentation).

Originally it looked like this: (from https://github.com/angular-ui/bootstrap/blob/master/src/pagination/pagination.js#L161-L172)

// Add links to move between page sets
if ( isMaxSized && ! rotate ) {
    if ( startPage > 1 ) {
        var previousPageSet = makePage(startPage - 1, '...', false);
        pages.unshift(previousPageSet);
    }

    if ( endPage < totalPages ) {
        var nextPageSet = makePage(endPage + 1, '...', false);
        pages.push(nextPageSet);
    }
}

function getPages(currentPage, totalPages) creates an empty array named pages, adds page number links, then adds links to move between page sets, then returns pages. This is an array that contains the number of pages shown (depends on maxSize) plus any ellipses (links to move between page sets). Each link when clicked just calls getPages() again to get a new set of page links with a different currentPage.

Every object added to pages looks like:

{
    number: number,
    text: text,
    active: isActive
}

I'm not sure where the "doubly linked list that loops at the ends, like a carousel is". Are you talking about a data structure or a UX concept?

The code in this PR just adds a couple more additional links to move between page sets (first and last page numbers and ellipses in more cases). These new links are just links that change currentPage and call getPages(). Granted I understand that the conditions I added to figure out when to add these new links became complex but that is not the same complexity that you are describing getting rid of.

Last thought about UX

If we enable first and last links containing page numbers there needs to be something separating them from the rest of the numbers when they are not sequential. Currently in the PR force-ellipses provides that separation. Even if force-ellipses is removed, boundary-link-numbers will probably still need to use that functionality.

Now I was looking at other sites like StackOverflow for ideas.

The current link is always centered but the ellipses aren't links. We could do that instead (make the ellipses separators and not links) for when rotate and boundary-link-numbers are true if that would make things better.

[wesleycho]

Ah, my apologies, I was completely confused - I thought it was supporting moving from [insert last page] to page 1. I am ok with what you are suggesting.

Still, I believe this should be split off into two separate PRs, and should not have the commit pollution.

[OzzieOrca]

Alright. Glad we are on the same page. I'll work on that. I might not get back to it immediately.

[OzzieOrca]

Actually I did already kinda split them. This is dependent on #3564 which adds the force-ellipses option only. Ideally that should have been merged before starting the boundary-link-numbers feature in the current PR but they both exist now.

The old commit by @wtc-dstone still needs to be dealt with though in both of them. Should I just do a rebase to squash the first 2 commits?

[wesleycho]

History needs to be fixed - there should not be a merge commit.

[OzzieOrca]

I did a rebase instead of a merge. I still don't know what to do with the commit by @wtc-dstone.

There's this tool to change author info but that seems exactly like stealing code. I could squash that code into one of my commits but that seems the same.

His code was in #3035 so it seems like he intended to allow this commit to be part of this project.

I think I need direction from you since this is a licensing issue.

[wesleycho]

Looks like you are correct after reading the comments through the issue & PR.

I am fine with his/her commit being there.

I need to take another look at this PR.

[wesleycho]

This needs rebasing.

[OzzieOrca]

Done.

[wesleycho]

Can you squash the last 3 commits into 1? I will look into getting this in when that is done - this will make rebasing on top of master on my end much smoother.

[OzzieOrca]

Done.