[DDW-360] Transactions paging #1078
Conversation
thedanheller
requested review from
nikolaglumac,
DominikGuzei and
MarcusHurney
source/renderer/app/api/ada/index.js
Outdated
|
|
||
| // In case there is more than one page, | ||
| // it loops through them and adds to the `history` array | ||
| if (totalPages > 1) { |
There was a problem hiding this comment.
@daniloprates in case we need just 5 transactions we don’t need to fetch more than 1 page
source/renderer/app/api/ada/types.js
Outdated
| data: Array<AdaTransactionV1>, | ||
| meta: { | ||
| pagination: { | ||
| totalPages: number, |
There was a problem hiding this comment.
@daniloprates please add the types for all other possible meta data props from the docs
There was a problem hiding this comment.
@daniloprates
I’ve got a type for meta data. You can get the entirety as ResponseBaseV1 or Pagination if you just want that. They are at the bottom of api/ada/types.js
There was a problem hiding this comment.
@daniloprates can you please check this out?
There was a problem hiding this comment.
@MarcusHurney great. @nikolaglumac done.
There was a problem hiding this comment.
Great work @daniloprates!
I have left some comments we need to resolve before merging.
|
@DominikGuzei please review this one as this is quite important change! Thanks! |
…0-transactions-paging
…0-transactions-paging
source/renderer/app/api/ada/index.js
Outdated
|
|
||
| // In case there is more than one page, | ||
| // it loops through them and adds to the `history` array | ||
| if (totalPages > 1 && limit > 50) { |
There was a problem hiding this comment.
You could remove the comment by using a descriptive var like const hasMultiplePages = (totalPages > 1 && limit > perPage) in the conditional instead of explaining what the expression does 😉
(in general if you find yourself explaining simple things like this in comments, try to find a more descriptive way to express the same idea in code)
source/renderer/app/api/ada/index.js
Outdated
| // In case there is more than one page, | ||
| // it loops through them and adds to the `history` array | ||
| if (totalPages > 1 && limit > 50) { | ||
| for (let page = 2; (page < totalPages + 1 && page <= pageLimit); page++) { |
There was a problem hiding this comment.
Same here, there is too much going on in this loop condition … you could simplify the condition by introducing two vars const hasNextPage = page < totalPages + 1 and const shouldLoadNextPage = page <= pageLimit
|
@DominikGuzei thanks for the great suggestions. |
This PR implements the transactions history paging.
Todo:
totalPagesto request the transactions from the next pageslimitparamObs.: It's possible to test the pagination by hardcoding the
perPagevariable to 1:if (perPage === 50) perPage = 1;Review Checklist:
Basics
yarn run test)yarn run dev)yarn run package/ CI builds)yarn run flow:test)yarn run lint)yarn run manage:translationsproduces no changes)yarn run storybook)yarn.lockfile is updatedCode Quality
Testing
After Review:
doneon the Youtrack board