search

GatsbyCentral / gatsby-awesome-pagination

An opinionated, more awesome approach to pagination in Gatsby
MIT License
110 stars 27 forks source link

Allow fine-tuning pathPrefix using a function #8

Closed silvenon closed 5 years ago

silvenon commented 5 years ago

This is the last thing I was missing from gatsby-awesome-pagination. pagePath is optional, it allows you to modify pages path:

paginate({
  createPage,
  items: blogPosts,
  itemsPerPage: perPage,
  pathPrefix: `/blog`,
  pagePath: pageNumber => `page/${pageNumber + 1}`,
  component: blogTemplate,
})

The above will result in following paths:

What do you think? Is the level of abstraction appropriate?

chmac commented 5 years ago

Interesting, thanks for the PR. I like the concept. I imagine this will support other use cases also.

I'm a big fan of the Phil Karlton quote "2 hard problems in computer science, cache invalidation and naming things". :-) I'm not sure if pagePath tells me immediately that this is a helper function. In general I love names like getFoo() for getters, etc. I'm not sure how that translates here though.

I'll do some thinking on naming and get back with a more useful reply next week. Wanted to fire this off quickly to say thanks for the PR, I really appreciate it.

silvenon commented 5 years ago

I'm also not a big fan of the name pagePath, especially because it collides with paginatedPath. I'll also rethink this.

chmac commented 5 years ago

@silvenon How about if we allow pathPrefix to be a function? Then we can pass it some variables and it can decide what the prefix should be for any given page.

For example:

paginate({
  createPage,
  items: blogPosts,
  itemsPerPage: perPage,
  pathPrefix: ({pageNumber}) => pageNumber === 0 ? "/blog" : "/blog/page",
  component: blogTemplate,
})

This would support your use case with the existing API naming. I imagine that pathPrefix receives an object containing the keys pageNumber and numberOfPages.

silvenon commented 5 years ago

I like this idea, it's really intuitive, I would only add one adjustment:

pathPrefix: ({pageNumber}) => pageNumber === 0 ? "/blog" : `/blog/page/${pageNumber + 1}`,

In my mind this is more transparent and flexible. What do you think?

silvenon commented 5 years ago

Actually, then the name pathPrefix would no longer make sense… I like your idea better.

silvenon commented 5 years ago

Done! I updated the documentation and added tests for paginate function. Down the road you can add some for createPagePerItem as well.

chmac commented 5 years ago

@silvenon Awesome, thanks a lot. Especially for the tests.

I was messing around a bit on master and something produced a big ugly merge conflict in yarn.lock. I copied your branch, rebased, and made a few small amendments. Mostly just style stuff, I tweaked the docs, added some comments, split the type def into 2, and applied prettier.

I don't think I can push my changes into your branch. Plus the rebasing would mess with your history. To that end I've created a new PR for the #9. I've also added contributors to the README and pushed those.

If you're happy with #9 in favour of this, I'll close here and merge there.

silvenon commented 5 years ago

Looks good! Feel free to use that one instead. 😉

Btw, Yarn is good at auto-resolving merge conflicts in yarn.lock. If you get a merge conflict, I think you just run yarn and it resolves the lockfile by itself.

chmac commented 5 years ago

Ooooh, nice, I didn't know that about yarn. Thanks. 👍

I'll merge #9.