feat: Added Pagination to the Resources Page #166
Conversation
|
YAY thank you for putting up this draft, @pdotsani! Do you need help getting the merge conflicts fixed? Separately, I'll tag this as |
|
Also, I won't review this until this is out of draft mode. |
pdotsani
force-pushed
the
feature/resources-pagination
branch
from
16f459b to
02c291c
Compare
pdotsani
force-pushed
the
feature/resources-pagination
branch
from
02c291c to
4c54a76
Compare
|
@lpatmo Looks like a rebase fixed all the conflicts. Should be good for a review. |
| @@ -0,0 +1,95 @@ | |||
| import React, { useState } from 'react'; | |||
There was a problem hiding this comment.
I think this file can be removed in favor of src/pages/Resources/Resources.js, right?
src/pages/Resources/Resources.js
Outdated
| getResources | ||
| ); | ||
| const [goToPage, setGoToPage] = useState(); | ||
| const [isPagination, triggerPagination] = useState(); |
There was a problem hiding this comment.
Was tempted to ask that this be renamed paginate, setPaginate for consistency's sake, but triggerPagination has a certain ring to it 🤔
There was a problem hiding this comment.
Also, thinking we should initialize the value to be null: useState();
src/pages/Resources/Resources.js
Outdated
| @@ -58,6 +67,18 @@ function Resources() { | |||
| </Typography> | |||
| )} | |||
| <br /> | |||
| <Pagination | |||
There was a problem hiding this comment.
Should we also hide the pagination UI when there is a searchValue?
That way we won't see something like this:
That "1" is clickable, leading to this UI which doesn't make sense:
src/pages/Resources/Resources.js
Outdated
| @@ -58,6 +67,18 @@ function Resources() { | |||
| </Typography> | |||
| )} | |||
| <br /> | |||
| <Pagination | |||
There was a problem hiding this comment.
UI nitpick: what do you think about moving the pagination component to the bottom of the grid? :)
|
🎉 Functionality works and you've figured out how to work with https://material-ui.com/components/pagination/, which is great! Left a few comments re: minor details like:
Thanks @pdotsani! Happy to discuss more in threads if you want to push back or have questions |
| let params; | ||
|
|
||
| if (isPagination) { | ||
| params = `?page=${goToPage}`; | ||
| } else { | ||
| params = `?search=${searchValue}`; | ||
| } |
There was a problem hiding this comment.
| let params; | |
| if (isPagination) { | |
| params = `?page=${goToPage}`; | |
| } else { | |
| params = `?search=${searchValue}`; | |
| } | |
| const params = isPagination? `?page=${goToPage}` : `?search=${searchValue}`; |
…agination component when there is no search term, and moved it down on the page
What type of PR is this? (check all applicable)
Context
Closes issue #104
Screenshots/Recordings (if there are UI changes)
Implementation Details - what was your thought process as you changed the code?
Added tests?
Added to documentation (readme.md or contributing.md)?