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

Align previous/next page flag with relay standard. #225

Merged
merged 3 commits into from
Apr 24, 2023

Conversation

rafa-avila-bc
Copy link
Contributor

This is related to graphql/graphql-relay-js#58 and bi-directional pagination.

I know that the changes in Connection.connectionFromSeq are kind of cumbersome so any help on improving those would be very much appreciated.

Also, I tried to document why the changes in the tests with small comments just to clarify (and honestly, those comments helped me to debug the offsets computations).

Thanks in advance.

@yanns
Copy link
Contributor

yanns commented Apr 23, 2023

Thanks for the contribution.
Can you check the compilation error on older scala version: https://github.com/sangria-graphql/sangria-relay/actions/runs/4767885408/jobs/8494878854?pr=225 ?

@rafa-avila-bc
Copy link
Contributor Author

@yanns Thanks for the FYI. I've addressed the incompatibilities. 🙇🏾‍♂️

Copy link
Contributor

@yanns yanns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't use relay, so I trust you on this.
From the code perspective, everything is looking good. I let a minor suggestion. Feel free to pick it or not.

src/main/scala/sangria/relay/Connection.scala Outdated Show resolved Hide resolved
@yanns yanns added this pull request to the merge queue Apr 24, 2023
@yanns
Copy link
Contributor

yanns commented Apr 24, 2023

Does this PR fix #35?

@yanns
Copy link
Contributor

yanns commented Apr 24, 2023

And #11?

Merged via the queue into sangria-graphql:main with commit 21e0c95 Apr 24, 2023
@rafa-avila-bc
Copy link
Contributor Author

This PR fixes #35
Regarding, #11 it seems to me that those cases were already handled correctly prior to this PR. This test is related.

@yanns
Copy link
Contributor

yanns commented Apr 25, 2023

Thanks a lot for the help!!

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