-
Notifications
You must be signed in to change notification settings - Fork 0
Paginated Survey Lists #96
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
base: development
Are you sure you want to change the base?
Conversation
|
Task linked: CU-8697zzruu Change forms to paginated forms |
|
@9sneha-n could you merge development into this branch to solve the conflicts? thank you! |
|
@MiquelAdell Merge done. |
MatiasArriola
left a comment
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 @9sneha-n! Great work overall, this was a big change! I left some comments for improvements. Let me know if if you have any question or if you'd like to discuss anything further.
Another idea to consider: instead of passing all the paging and sorting state setters/getters down from SurveyListPage -> SurveyList -> PaginatedSurveyListTable , you could use a Context and access the sorting and paging state via custom hooks within the components. This could help simplify prop drilling. I'm not sure if this change is necessary for approval, but I wanted to suggest it as a possible improvement. It's up to you to decide if it's valuable and whether we have time to implement it, as it might not be a straightforward change.
Thanks again!
src/webapp/components/survey-list/table/PaginatedSurveyListTable.tsx
Outdated
Show resolved
Hide resolved
src/webapp/components/survey-list/table/PaginatedSurveyListTable.tsx
Outdated
Show resolved
Hide resolved
Good idea @MatiasArriola . @MiquelAdell the proposed design change is good to have for improved code. Based on our discussion in follow-up, wanted to check if we have time for this enhancement at this point in time? If not, we can add a task for "Create survey list page details context" in the backlog. |
|
@MatiasArriola Addressed comments, ready for re-review. |
MatiasArriola
left a comment
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.
All good, thanks @9sneha-n !!
📌 References
📝 Implementation
📹 Screenshots/Screen capture
🔥 Notes to the tester