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

fix: pagination credit batches #1906

Merged
merged 1 commit into from
May 23, 2023
Merged

Conversation

flagrede
Copy link
Collaborator

@flagrede flagrede commented May 22, 2023

Description

Closes: regen-network/rnd-dev-team#1662

Remove hidden from current pagination count.


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • provided a link to the relevant issue or specification
  • provided instructions on how to test
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

How to test

  1. Go to https://deploy-preview-1906--regen-registry.netlify.app/ecocredits/accounts/regen1df675r9vnf7pdedn4sf26svdsem3ugavgxmy46/portfolio
  2. navigate to page 21-24

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items
.

I have...

  • confirmed all author checklist items have been addressed
  • reviewed code correctness and readability
  • verified React components follow DRY principles
  • reviewed documentation is accurate
  • reviewed tests
  • manually tested (if applicable)

@flagrede flagrede requested a review from a team May 22, 2023 09:52
@flagrede
Copy link
Collaborator Author

@erikalogie you can check the pagination bug fix here: https://deploy-preview-1906--regen-registry.netlify.app/ecocredits/accounts/regen1df675r9vnf7pdedn4sf26svdsem3ugavgxmy46/portfolio
There's an hidden row on the page 21-24

@erikalogie
Copy link
Collaborator

@flagrede this looks good. As you said on slack the rest will be solved with the ledger upgrade

@wgwz
Copy link
Contributor

wgwz commented May 22, 2023

It's a little funky how "rows per page" is set to 5, but the page for 21-14 only has 4 results, and then the next page has 5 results again
I'm not against it working that way because it does seem to be working
But it's just a little unexpected and not what I'm used to with pagination

Screen Shot 2023-05-22 at 4 57 46 PM

@flagrede
Copy link
Collaborator Author

flagrede commented May 23, 2023

@wgwz You're missing some context for this issue I think. The initial issue was that the ledger is sending us ecocredits that used to have balance but do not anymore. We wanted to hide these "ghost" credits from the user so we filter them on the front end. This created an inconsistency with pages having "ghost" credits, we were having labels like "21-25" while displaying only 4 rows for example. This PR just takes into account those "ghost" credits to adjust the pagination label accordingly. Eventually, this issue will be solved on the ledger side, in the meantime we try to accommodate it as best as we can.

@blushi
Copy link
Member

blushi commented May 23, 2023

@wgwz see underlying issue on regen-ledger: regen-network/regen-ledger#1889

@wgwz
Copy link
Contributor

wgwz commented May 23, 2023

@flagrede @blushi thanks for the context, that makes sense now. indeed, it'll be best to fix that in the ledger. i have no problem with this in the mean time!

Copy link
Contributor

@wgwz wgwz left a comment

Choose a reason for hiding this comment

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

tACK

@flagrede flagrede merged commit 46fa9c8 into dev May 23, 2023
@flagrede flagrede deleted the fix-1662-pagination-credit-batches branch May 23, 2023 12:45
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.

4 participants