Skip to content

Less opinionated template#1

Open
Sinetheta wants to merge 2 commits intoTimHeckel:masterfrom
Sinetheta:less-opinionated
Open

Less opinionated template#1
Sinetheta wants to merge 2 commits intoTimHeckel:masterfrom
Sinetheta:less-opinionated

Conversation

@Sinetheta
Copy link
Copy Markdown

Removed "pull-right"s, the "results per page" div, and cleaned some whitespace. Those are probably best left to the user.

@TimHeckel
Copy link
Copy Markdown
Owner

I certainly agree with pulling the styles out and removing the results per page label div...along these same lines, it would make sense to allow users to populate the result per page values themselves (rppOptions: [5, 10, 15, 20, 30, 50, 100, etc])...any chance you'd like to add that to your PR ? I'll merge :)

@Sinetheta
Copy link
Copy Markdown
Author

Sure, but that leads to another discussion, deep options. Since the options hash is itself made up of objects and _.extend is not recursive, setting only "some" of a nested option will crush the others. For example:

pagination: { resultsPerPage: 5 //default limit }

as in your example will overwrite opts.pagination.currentPage which makes the first _scheduledPage undefined and means it can't be incremented so that clicking "next" as your first action wouldn't work.

Those options should really be broken out on their own, instead of nested. But then that leads to the discussion about how much of this should be encapsulated within the Paginator instead of being expressed by the user in their templates, etc... /rabbit-hole

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.

2 participants