Conversation
|
Was this PR acceptable? I didn't receive any feedback on it |
|
Hi @ericsullivan! Thank you for your contribution and apologies for the lack of response to it thus far. I'll review it for you now. |
sgerrand
left a comment
sgerrand
left a comment
There was a problem hiding this comment.
The interface change looks fine to me, especially as this change is additive and should be backwards compatible. There's an open question inline in a test which needs to be resolved and removed before merging.
test/paginator_test.exs
Outdated
| # Question: Should this return a value? | ||
| # assert page.metadata.before == encode_cursor(%{charged_at: p5.charged_at, id: p5.id}) |
There was a problem hiding this comment.
Is your question "should the page return information about a previous page in the initial page"? Some clarification would help me to answer it.
There was a problem hiding this comment.
Yes, that was the question. If you're on the first page, before is nil. If a new record is added before the first page, you can't use the before to get to it. It feels like and edge case so I'm just going to remove the comment
|
When you have a chance, would you please rebase your changes against |
|
@ericsullivan, I've left some feedback for you and I'll wait on your response. I'll be available for review again later this week! |
|
👋 @ericsullivan, are you still interested in making this change? |
ericsullivan
force-pushed
the
feature/page_booleans
branch
from
907e235 to
b7b4505
Compare
|
Sorry, for the delay. I just rebased and removed the comment |
Co-authored-by: Sasha Gerrand <sgerrand@users.noreply.github.com>
ericsullivan
force-pushed
the
feature/page_booleans
branch
from
b7b4505 to
0a4e7ff
Compare
|
Hi. Any updates on this? |
3 participants
Relay Doc
resolves: #84