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

show_blocks pagination #73

Merged
merged 4 commits into from
Sep 21, 2014
Merged

Conversation

BooDoo
Copy link
Contributor

@BooDoo BooDoo commented Sep 20, 2014

Still need to resolve "Block All" behavior and use some less generic class names, but could you advise if I'm on the right path with how pagination behaves/is presented?

  • Get total count of blocks in a BlockBatch within showBlocks()
  • Fetch perPage (500) blocks at a time
  • Offset blocks fetch based on "?page=" querystring
  • Pass currentPage and pages[] to templates/show_blocks
  • pagination.mustache made for inclusion in show-blocks
  • CSS counters added to style.css for numbering pagination links
  • pagination.js handles detecting current page and navigating to others
  • Remove 'theres_more' flag

- Get total count of blocks in a BlockBatch within showBlocks()
- Fetch perPage (500) blocks at a time
- Offset blocks fetch based on "?page=" querystring
- Pass currentPage and pages[] to templates/show_blocks
- pagination.mustache made for inclusion in show-blocks
- CSS counters added to style.css for numbering pagination links
- pagination.js handles detecting current page and navigating to others
- Remove 'theres_more' flag
@jsha
Copy link
Owner

jsha commented Sep 20, 2014

This is looking awesome! One small thing: Instead of setting handlers on the various page links, how about setting their href to the target URL? That way things like middle clicking work correctly.

Let me know when it's ready for a full review, and thanks.

@BooDoo
Copy link
Contributor Author

BooDoo commented Sep 20, 2014

Hadn't done literal hrefs because mu2 provides no access to current element value (raycmorgan/Mu#10) but since I'm leaning on jQuery for a few other things I'll update hrefs after load in there.

@jsha
Copy link
Owner

jsha commented Sep 20, 2014

You might be able to get what you want out of Mustache with something like:

pages: _.range(1, Math.ceil(blocksCount / perPage) + 1).map(function(pageNum) {
return {
pageNum: pageNum,
isActive: pageNum === current
}
},

- Move pagination bar logic to blocktogether.js
- Omit previous/next links instead of disabling them
- More accurately label the "Block All" button for pagination
@BooDoo
Copy link
Contributor Author

BooDoo commented Sep 21, 2014

Ready for code review. "Block All" button hasn't been given special logic to block pages other than the currently visible one (client side JS not touched.)

Changes since first push:

  • Rename classes for pagination template
  • Hide pagination section if only one page is needed
  • Rename classes used in pagination template

jsha added a commit that referenced this pull request Sep 21, 2014
@jsha jsha merged commit c6477ff into jsha:master Sep 21, 2014
@jsha
Copy link
Owner

jsha commented Sep 21, 2014

Excellent change, thanks!

@BooDoo BooDoo deleted the issue/18/paginate-show-blocks branch September 21, 2014 21:12
@BooDoo
Copy link
Contributor Author

BooDoo commented Sep 25, 2014

So we can close #17 and #18 with this one?

@jsha
Copy link
Owner

jsha commented Sep 25, 2014

Closed out #18. I think you had mentioned fixing #17 with this change, but I don't see the changes to the code to make that happen?

@BooDoo
Copy link
Contributor Author

BooDoo commented Sep 25, 2014

I misread #17's intent in my notes! This PR actually exacerbates it, not solves it.

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