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

Add pagination to the "Pending Owner Invites" page #5185

Closed

Conversation

EstebanBorai
Copy link
Contributor

@EstebanBorai EstebanBorai commented Sep 2, 2022

Provides the missing pagination for the Pending Owner Invites page
on /me/pending-invites.

Refer: #4083


Demo

demo.mov

Currently the `crate_id` which is a `string` with the crate's name
is attempted to be parsed on serialization, causing trouble at the
moment of fetching crates in fixtures db.
Comment on lines -27 to +29
hash.crate_id = Number(hash.crate_id);
// TODO: Check this further, `crate_id` are strings in the current fixtures
// when attempting to parse into number we will get `NaN`s here.
// hash.crate_id = Number(hash.crate_id);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is causing some trouble when retrieving crates by its ID, given that crates IDs are actually strings with the name of such crate.

@EstebanBorai EstebanBorai marked this pull request as ready for review September 6, 2022 04:49
@EstebanBorai EstebanBorai marked this pull request as draft September 6, 2022 04:56
@EstebanBorai
Copy link
Contributor Author

I'm taking this back to draft as I still have to check for backend pagination support

@Turbo87 Turbo87 added C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works A-frontend 🐹 labels Sep 9, 2022
@EstebanBorai EstebanBorai force-pushed the fix/owner-invites-pagination branch from d48c43e to 78d249b Compare September 13, 2022 01:18
@EstebanBorai
Copy link
Contributor Author

Hi @Turbo87!

Im working on providing support for the seek pagination format on the pagination component.
And I notice we have a fragment already wrote for the next_page which comes handy for the next button.
But when it comes for page numbers, I'm thinking of possible approaches.

As I see in this fragment

let mut params = IndexMap::new();

As we must know the ids of the last entry in the current page in order to fetch the next page (or a specific one), I was thinking about retrieving next sets as part of the meta included in the response. For instance if you fetch page 1 and establishing that we will always have a range of 10 pages per view. I would retrieve a pages property with the corresponding ID tuples for the next 10 pages.

What do you think? Perhaps we already have a definition for this?

Thanks in advance!

@EstebanBorai EstebanBorai force-pushed the fix/owner-invites-pagination branch from 36c2d62 to b83213c Compare September 14, 2022 19:37
@EstebanBorai EstebanBorai force-pushed the fix/owner-invites-pagination branch from b83213c to 80db2b4 Compare September 14, 2022 19:40
@EstebanBorai
Copy link
Contributor Author

Moved into just next page support due to the behavior of the seek pagination.
The following demo shows how seek pagination is used to load the next page. In a next iteration it would be great (IMHO) to be able to load next invites in the same page, similar to an infinity scroll behavior.

Demo

Screen.Recording.2022-09-14.at.4.30.42.PM.mov

@EstebanBorai
Copy link
Contributor Author

Also noticed the button on gray was a bit "misleading" based on the color of remaining anchors around the site.
So I fixed it using the same green as we always do.

Screen Shot 2022-09-14 at 4 40 25 PM

@EstebanBorai EstebanBorai marked this pull request as ready for review September 15, 2022 18:00
@Turbo87 Turbo87 changed the title fix: pending owner invites pagination Add pagination to the "Pending Owner Invites" page Sep 18, 2022
@EstebanBorai EstebanBorai marked this pull request as draft September 18, 2022 14:15
Comment on lines +3 to +12
export function pagination() {
return macro(function () {
const { nextPage } = this;
const nextPageParams = new URLSearchParams(nextPage);

return {
nextPage: nextPageParams.get('seek'),
};
});
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Parses the next_page value from the response meta field which comes a URL Query Params, and retrieves the value from seek (if available) otherwise returns undefined.

@@ -47,6 +49,7 @@ pub fn list(req: &mut dyn RequestExt) -> EndpointResult {
Ok(req.json(&json!({
"crate_owner_invitations": crate_owner_invitations,
"users": users,
"meta": meta,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adds meta to the response similar to how its done for crates endpoint, this way we can retrieve the seek hash for the next page.

Copy link
Member

Choose a reason for hiding this comment

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

sorry that I just noticed this now, but #3763 introduced a new endpoint for the invitations (GET /api/private/crate_owner_invitations) that already supports seek-based pagination. I guess it would be best if we first switched to this new endpoint in the frontend in a dedicated MR and then implemented pagination on top of it. let me know if you want to tackle this :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good! Lets do that!
So we close this PR?

Copy link
Member

Choose a reason for hiding this comment

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

whatever you prefer. feel free to keep it open and then rebase it once the other stuff is done :)

@EstebanBorai EstebanBorai marked this pull request as ready for review September 19, 2022 00:16
@bors
Copy link
Contributor

bors commented Oct 23, 2022

☔ The latest upstream changes (presumably #5370) made this pull request unmergeable. Please resolve the merge conflicts.

@EstebanBorai
Copy link
Contributor Author

Closing and moving into another pr with the new seek based pagination...

@EstebanBorai EstebanBorai deleted the fix/owner-invites-pagination branch October 28, 2022 04:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-frontend 🐹 C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants