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

CIDs paging [storage] #1889

Merged
merged 34 commits into from
Apr 25, 2022
Merged

CIDs paging [storage] #1889

merged 34 commits into from
Apr 25, 2022

Conversation

tanmoyAtb
Copy link
Contributor

@tanmoyAtb tanmoyAtb commented Jan 28, 2022

closes #1325

Some functionality to look into
Move into next pages,
Move into previous pages,
Search Pins,
Add pin,
Unpin


Submission checklist:

Layout

  • Change looks good in the desktop web ui

Theme

  • Components / elements inspected in light mode

@render
Copy link

render bot commented Jan 28, 2022

@render
Copy link

render bot commented Jan 28, 2022

@render
Copy link

render bot commented Jan 28, 2022

@github-actions github-actions bot added the Type: Feature Added to PRs to identify that the change is a new feature. label Jan 28, 2022
@lgtm-com
Copy link

lgtm-com bot commented Jan 28, 2022

This pull request introduces 3 alerts when merging ce5c1f4 into a08b19d - view on LGTM.com

new alerts:

  • 3 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented Mar 11, 2022

This pull request introduces 3 alerts when merging 7a2a114 into 03f67c5 - view on LGTM.com

new alerts:

  • 3 for Unused variable, import, function or class

@tanmoyAtb tanmoyAtb marked this pull request as ready for review April 13, 2022 16:42
@tanmoyAtb tanmoyAtb requested review from Tbaut, FSM1 and asnaith April 13, 2022 16:45
Copy link
Contributor

@FSM1 FSM1 left a comment

Choose a reason for hiding this comment

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

The code looks really great. The only comment I have is when switching pages, both the buttons become disabled (which is expected), and a spinner appears in both of the page buttons (this is less expected).

Not sure what the solution is here - either show a spinner only on the button that was clicked, or do not have a spinner on the button during the request at all.

CIDs.-.Chainsafe.Storage.-.Brave.2022-04-14.17-11-51.mp4

@tanmoyAtb
Copy link
Contributor Author

The code looks really great. The only comment I have is when switching pages, both the buttons become disabled (which is expected), and a spinner appears in both of the page buttons (this is less expected).

Not sure what the solution is here - either show a spinner only on the button that was clicked, or do not have a spinner on the button during the request at all.

CIDs.-.Chainsafe.Storage.-.Brave.2022-04-14.17-11-51.mp4

All right, we're handling to show spinner only on the next or previous button now. 👍

@tanmoyAtb tanmoyAtb requested a review from FSM1 April 15, 2022 08:19
@asnaith
Copy link
Member

asnaith commented Apr 18, 2022

Hey @tanmoyAtb, I found an issue that I've demonstrated in the video below. I had two pages of cids but after sorting one of the columns the paging system broke.

breaking.paging.mov

@tanmoyAtb
Copy link
Contributor Author

Hey @tanmoyAtb, I found an issue that I've demonstrated in the video below. I had two pages of cids but after sorting one of the columns the paging system broke.

breaking.paging.mov

Ahh yes, that makes sense, That's a great catch @asnaith. I'll be looking into it.

@tanmoyAtb tanmoyAtb changed the title CIDs paging CIDs paging [storage] Apr 18, 2022
@tanmoyAtb
Copy link
Contributor Author

Hey @tanmoyAtb, I found an issue that I've demonstrated in the video below. I had two pages of cids but after sorting one of the columns the paging system broke.

breaking.paging.mov

The sort directions should work properly now. @asnaith

Copy link
Collaborator

@Tbaut Tbaut left a comment

Choose a reason for hiding this comment

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

I've had a hard time concentrate to properly understand the logic happening here, I'm particularly confused by the dates, I bet this is what we base our sorting on, rather than any id, also flags like isPreviousPin, were confusing maybe it's a naming thing.
In any case I made a couple suggestions, but playing with it was flawless 🚀

packages/storage-ui/src/Components/Pages/CidsPage.tsx Outdated Show resolved Hide resolved
packages/storage-ui/src/Contexts/StorageContext.tsx Outdated Show resolved Hide resolved
packages/storage-ui/src/Contexts/StorageContext.tsx Outdated Show resolved Hide resolved
@tanmoyAtb
Copy link
Contributor Author

I've had a hard time concentrate to properly understand the logic happening here, I'm particularly confused by the dates, I bet this is what we base our sorting on, rather than any id, also flags like isPreviousPin, were confusing maybe it's a naming thing. In any case I made a couple suggestions, but playing with it was flawless 🚀

Agreed, Going to improve the variable names here.

So with the dates. There has been a lot of back and forth with this logic to get here and its a bit tricky.
To fetch next pins -> we pass a before date based on the oldest pin date available in view, we fetch them in date descending order, so the we get pins closest to the before date.
For previous pins -> We pass an after date based on the latest pin in view, and fetch in date ascending order, again to get the latest pins closest the the after date.

@tanmoyAtb tanmoyAtb enabled auto-merge (squash) April 25, 2022 14:59
@tanmoyAtb tanmoyAtb merged commit 325820a into dev Apr 25, 2022
@tanmoyAtb tanmoyAtb deleted the feat/cids-page-1325 branch April 25, 2022 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Product: Storage Type: Feature Added to PRs to identify that the change is a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add paging to CIDs page
4 participants