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

adding pagination to bill index page #275

Merged
merged 7 commits into from
Mar 8, 2022

Conversation

0lafe
Copy link
Collaborator

@0lafe 0lafe commented Mar 5, 2022

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

@github-actions
Copy link

github-actions bot commented Mar 5, 2022

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

@jkoren
Copy link
Collaborator

jkoren commented Mar 6, 2022

This looks really nice paginating through bills when they are in bill order!
I did however get an error when going to page 2 of bills when they are in # of cosponsors order

image

Copy link
Member

@alexjball alexjball left a 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

@@ -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}}>
Copy link
Member

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

onClick={previousPage}
disabled={currentPage === 1}
>
{"<"}
Copy link
Member

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

https://fontawesome.com/v5/icons/angle-left?s=solid

@alexjball
Copy link
Member

@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

@jkoren
Copy link
Collaborator

jkoren commented Mar 8, 2022

@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
const {member, loading} = useMember(bill.PrimarySponsor ? bill.PrimarySponsor.Id : null)
Yeah let's discuss amended versions of bills.

@0lafe
Copy link
Collaborator Author

0lafe commented Mar 8, 2022

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 hasNextPage value from useBills seems to not always be accurate. When sorting by # of Testimonies there are only 4 total, but I can get to page 3 with the same results each page. Sorting by Recent Testimonies will let me go to page 2 which is blank

Copy link
Member

@alexjball alexjball left a 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.

@alexjball alexjball merged commit 4fc6e42 into codeforboston:master Mar 8, 2022
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.

Add button to Bills page for pagination
3 participants