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

Hide redundant Page Size buttons #648

Closed
wants to merge 2 commits into from
Closed

Hide redundant Page Size buttons #648

wants to merge 2 commits into from

Conversation

enkodellc
Copy link
Contributor

Add logic to show only page size if the row count is greater than the count and less than the next count button.

@enkodellc
Copy link
Contributor Author

Sorry got a typo in my first commit. Either way TravisCI passed but CicleCI didn't? I followed the contribution guidelines. Not sure what else to do. Admin please let me know.

@christianacca
Copy link
Collaborator

There some overlap between this pull request and #656

Lots of good ideas. However, hiding page size buttons is probably not suitable in every situation.

As explained in the the other pull request, we might need to introduce a new option:

.. it could be a good time to consolidate the existing options determining how the pager is displayed. Once consolidated they would have more "room to grow".

Also, what I think is also important is not to load up on options as this can make it harder to work with the library.

Alternative solution:

  1. make it easier to create custom pager templates; AND/OR
  2. create another template, possibly 2 that will cover the main variations that apps require

@christianacca
Copy link
Collaborator

There's a pull request to implement infinite scroll (#560)

As commented:

... separate out pager options and page generation logic from NgTableParams
... someone to take the lead on this

@christianacca
Copy link
Collaborator

So looks like we have a bunch of options for pagination: #556, #555, #648 (this one), #656, #560, #454

@enkodellc
Copy link
Contributor Author

@christianacca I like your ideas you posted on #656 did you want to assign anyone to start doing those PR's? I am willing to do some work to join these PR's if you are willing to give me some support to get them done to be merged.

@christianacca
Copy link
Collaborator

Cool. When do you want to start on this? The first thing is to do the consolidation work:

Pull request 1 - refactor pagination settings (#656)

I'll can do a quick peer review and merge.

If you still got time, we can get onto the enhancements. If you stop there, your contribution will still be of benefit to the project.

C

@enkodellc
Copy link
Contributor Author

Ok I will look into the refactor for the pagination settings. Should I refactor for the current settings to start and then do additional PR's when adding features or should I include some of those settings right off the get go.

@christianacca
Copy link
Collaborator

Let's keep the refactoring into a separate PR

@enkodellc
Copy link
Contributor Author

I just submitted a separate PR. #674

@christianacca
Copy link
Collaborator

Yup, just merging it with some stuff I've just pushed. Also, I'm got a couple of small feedback items around the commit

@enkodellc enkodellc closed this Nov 27, 2015
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

2 participants