-
Notifications
You must be signed in to change notification settings - Fork 60
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
Add frontend support for Manage Users pagination #8371
base: main
Are you sure you want to change the base?
Conversation
52233eb
to
31b2d0a
Compare
c3ecb82
to
3a6b61e
Compare
31b2d0a
to
b2174fb
Compare
3a6b61e
to
d3166f1
Compare
21a5f9b
to
86fc1ca
Compare
cc6a935
to
c7c7652
Compare
c7c7652
to
e6a3da8
Compare
boolean requestedPageOutOfBounds = startIndex > totalSearchResults; | ||
|
||
if (onlyOnePageOfResults || requestedPageOutOfBounds) { | ||
startIndex = 0; |
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.
Had to handle this edge case because I encountered an error when I was on page 7 of all results and entered a search query. The app requested page 7 of the search results and the backend calculated the start index as a higher index than the end index. This caused an illegal argument error when attempting to create the sublist
Hmm I'll check with product and design to see what the intended behavior should be in this scenario. Right now the component determines the current page based on the url parameter so in that example the url param would still be 7 even when the search results returned just one page. We could possibly have the frontend trigger a redirect to the 1st page whenever a search is entered. I remember we discovered that focus redirect bug on the results page when we were pairing on this |
In a (probably unlikely) scenario where the results return too many patient names to fit on one page, how would the user navigate to "page 2"? (Ex: There are 20+ Wallaces.) Is there a large lift to have the pagination reflect the results filter? Alternatively, I'd want to know if pagination could be disabled so it does not imply the user can navigate between pages if the filter is active. cc @kenieh for thoughts |
Yes this type of paginated search should already be supported! So if you were to search by a single character like "a" it'll show multiple pages of filtered results |
@khayeni-SL @kenieh I updated dev5 with the styling changes and fixed the bug @emyl3 found. The search is now handled with the url query parameter |
Quality Gate passedIssues Measures |
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.
Left a couple nits and one thing I noticed based on our search implementation (Sorry I didn't notice this earlier when you introduced the implementation. 😓)
Looks like this search behaves slightly differently than our other ones.
search.functionality.mov
Let me know what you decide to do. Everything else looks good and works as expected for me.
<div className={"padding-1 padding-left-3 padding-bottom-4"}> | ||
<Button | ||
className={"clear-filter-button"} | ||
id={`clear-filter-button`} |
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: do we need this id
🤔
@@ -29,7 +29,7 @@ const Settings = () => { | |||
path={"self-registration"} | |||
element={<ManageSelfRegistrationLinksContainer />} | |||
/> | |||
<Route path="/" element={<ManageUsersContainer />} /> | |||
<Route path="users/:pageNumber" element={<ManageUsersContainer />} /> |
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.
I forgot we could also add something like the following:
<Route index element={<Navigate replace to="users/1"/>}/>
to default it to the users page like before if someone were to navigate to /settings
FRONTEND PULL REQUEST
Related Issue
Changes Proposed
settings/users/:pageNumber
so that the page number can be retrieved from the url paramsTesting
Access organization account
from the support admin dashboardBig Organization
, type in "test" for justification, and clickAccess data
Screenshots
Page 1 of results
Page 5 of results
Last page of results
Search results with multiple pages
Search results with single result page
No results found