-
-
Notifications
You must be signed in to change notification settings - Fork 110
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
adding pagination to bill index page #275
Conversation
Visit the preview URL for this PR (updated for commit 0lafe/advocacy-maps@c9c73e0): https://digital-testimony-dev--pr275-bill-pagination-aht0o9ec.web.app (expires Tue, 15 Mar 2022 23:08:20 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: bc0858669d4997df2a9165c2144bd1e2dbba0242 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Ben! I will push up a commit to this PR that adds hasNextPage
/ hasPreviousPage
props to useBills
, which you can use to enable/disable the buttons
components/ViewBills/ViewBills.js
Outdated
@@ -104,6 +105,25 @@ const ViewBills = (props) => { | |||
<Row> | |||
{loading && <Spinner animation="border" className="mx-auto"/>} | |||
</Row> | |||
<div className="d-flex justify-content-center" style={{marginBottom: 15}}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: you can add mb-3
to the className list rather than setting the margin in the style. bootstrap provides a bunch of utility classes to avoid having to define our own
components/ViewBills/ViewBills.js
Outdated
onClick={previousPage} | ||
disabled={currentPage === 1} | ||
> | ||
{"<"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use icons here rather than text. Check out links.tsx for how to import/use the fontawesome icons. I think we want to use faAngleRight and faAngleLeft
@0lafe added hasNextPage and hasPreviousPage props to useBills @jkoren The error on page 2 with the cosponsor sort seems to be due to PrimarySponsor being null for some of the bills, causing this line to fail. They all seem to be amended versions of bills (which is a whole other discussion as to how we should handle those -- I would think we should bundle them all together). H4470 and S2541 |
Sounds good. I believe we could fix the page 2 error by changing the code to |
I added the line jeff recommended, which does avoid the error. However this also excludes any bills that don't have a cosponsor from that sort. Not sure what we would want to do about that Also noticing that the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, Ben! I filed #287 to track fixing excluding bills without primary sponsors.
Fixes #269
Screen.Recording.2022-03-04.at.11.12.14.AM.mov
Needs a way to disable the forward button when reaching the end of the list. I'm not sure how to do that