refactor: Use Pagination component in react-uswds #897
Conversation
jcbcapps
requested review from
a team,
abbyoung,
esacteksab,
gidjin and
minhlai
as code owners
|
This pull request has been linked to Shortcut Story #747: Use Pagination component in react-uswds instead of our own. |
src/pages/about-us/orbit-blog.tsx
Outdated
| @@ -84,7 +84,7 @@ export const getServerSideProps: GetServerSideProps = async ( | |||
| }) | |||
| // Calculate total pages | |||
| // If NaN, return 1 page | |||
| const totalPages = Math.ceil(articlesCount / articlesPerPage) || 1 | |||
| const totalPages = Math.floor(articlesCount / articlesPerPage) || 1 | |||
There was a problem hiding this comment.
This determines the number of pages to display in the Pagination component. If the number of published articles was exactly 10 (the number of articles per page, specified in a constant on line 72) Math.ceil was causing the calculation to round up, creating a page link in the component that just linked to an empty page. Changing this to Math.floor returns the greatest integer less than or equal to its numeric argument, fixing the issue.
There was a problem hiding this comment.
Suggestion: I did some testing this seems to not be an issue because of Math.ceil but the fact that the graphql query articlesCount near line 75 is returning the total number of articles, regardless of their status. I had 12 in my db and page 2 didn't show up. I marked them as draft and the total count was still 12. So I've pushed a commit up that updates the query filter things down to just published and puts Math.ceil back in since with 12 articles we want 2 pages and 12/10 is 1.2.
There was a problem hiding this comment.
Probably worth eventually adding an e2e test for this, unless there are some orbit-blog unit tests around. Is this something we can test in src/__tests__/pages/about-us/orbit-blog.test.tsx? I would think mocking out the query with the where info and checking it's called at least would be enough, if it's possible.
There was a problem hiding this comment.
Good catch on the articlesCount! I'm not seeing any orbit-blog tests anywhere, but I agree we should test somehow. I can add that test file for the orbit-blog and mock out the query like you mentioned. Do you want me to also create a story in the backlog for an e2e test for pagination?
There was a problem hiding this comment.
Wanted to add, because I just found it, there is ArticleList.test.tsx that tests the pagination display. But the numbers are passed in manually via props. So I still don't think there is anything testing the actual query.
There was a problem hiding this comment.
Bummer, I think an e2e test would be good enough especially since that will test a real query. Though I would want to wait for the seed database work to be done before starting it since we'll need enough data to show the pagination. Long way to say yes please add a story to create an e2e test for this.
src/pages/about-us/orbit-blog.tsx
Outdated
| @@ -84,7 +84,7 @@ export const getServerSideProps: GetServerSideProps = async ( | |||
| }) | |||
| // Calculate total pages | |||
| // If NaN, return 1 page | |||
| const totalPages = Math.ceil(articlesCount / articlesPerPage) || 1 | |||
| const totalPages = Math.floor(articlesCount / articlesPerPage) || 1 | |||
There was a problem hiding this comment.
Suggestion: I did some testing this seems to not be an issue because of Math.ceil but the fact that the graphql query articlesCount near line 75 is returning the total number of articles, regardless of their status. I had 12 in my db and page 2 didn't show up. I marked them as draft and the total count was still 12. So I've pushed a commit up that updates the query filter things down to just published and puts Math.ceil back in since with 12 articles we want 2 pages and 12/10 is 1.2.
src/pages/about-us/orbit-blog.tsx
Outdated
| @@ -84,7 +84,7 @@ export const getServerSideProps: GetServerSideProps = async ( | |||
| }) | |||
| // Calculate total pages | |||
| // If NaN, return 1 page | |||
| const totalPages = Math.ceil(articlesCount / articlesPerPage) || 1 | |||
| const totalPages = Math.floor(articlesCount / articlesPerPage) || 1 | |||
There was a problem hiding this comment.
Probably worth eventually adding an e2e test for this, unless there are some orbit-blog unit tests around. Is this something we can test in src/__tests__/pages/about-us/orbit-blog.test.tsx? I would think mocking out the query with the where info and checking it's called at least would be enough, if it's possible.
There was a problem hiding this comment.
As per the intro page, I think we still need to track this component in Happo because we want to be notified if any changes in global CSS results in unintended visual changes. I would say to refactor the stories pages, but not get rid of them.
I would move your original pagination.stories.tsx file into src/stories and change the output to use the imported react uswds component. this will also ensure the styling isn't getting changed by importing it from react uswds. in this branch's currnt state, we are deleting the component out of storybook, and in turn happo, so we can't tell if the component was impacted, and we will lost the ability to track future changes going forward.
SC-747
Proposed changes
With the merging of this PR into the ReactUSWDS library, we ported over the
Paginationcomponent developed for this project. This PR removes thePaginationcomponent and imports it from the ReactUSWDS library.Reviewer notes
Run the app locally and navigate to
/about-us/orbit-blogand make sure that you have at least 10ORBITBlogarticles published in your local CMS. You should be able to see thePaginationcomponent at the bottom of the page.Bonus: a quick way to test out the different variations of the
Paginationcomponent, would be to adjust the value of thearticlesPerPagevariable on line 72 inorbit-blog.tsx. If you have a few articles published, then changing this number and refreshing the page will update the component.Setup
The usual. Make sure you are running both the CMS and the portal client locally.
Code review steps
As the original developer, I have
As code reviewer(s), I have
As a designer reviewer, I have
As a test user, I have
Screenshots