-
Notifications
You must be signed in to change notification settings - Fork 28
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
Tim add materials list routing #567
Conversation
… collection fetch
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.
Looks good! All functionality on frontend is working, and all information is showing up correctly on home page.
Only issues are editing seem to not be working, so I cannot test that functionality.
Screen.Recording.2023-10-11.at.2.49.16.PM.mov
Thanks for your review. As explained in the frontend PR description (which is where your change request should have been made since it is a frontend issue), the edit buttons currently return null by design. |
Looks good! All front-end functionality works as expected. Great job. I've left a video recording in the corresponding front-end PR. |
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.
Comment and test video left on FE PR. Great job!
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.
Hi I tested your PR, worked as expected, details in #1401
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.
Comment left on FE PR #1401.
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.
Hi @tdkent, I have tested your PR and everything seems to work perfectly. Left detailed review and test recordings on FE. Great job!!!
Comment left on FE PR #1401. |
Commented on the frontend PR |
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 main review on pr1401.
Would like to see chronological sort implemented in the mongodb query, so the results are return chronologically on the frontend.
@KurtisIvey Thanks for your review. I agree there should be sorting and additional filtering options. However, the PR was already so big I decided to stay focused on a MVP approach. I have already added this task to the Phase 2 WBS Sheet. For now I think its more important to get this branch merged since it adds many new file structure elements that are essential to other devs working on Phase 2.
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.
Clarification was given, so this things all good to go 👍🏻
Description
Backend updates to support the BM Dashboard materials list page.
Related PRS
Main changes explained:
GET
requestHow to test:
git checkout Tim_add_materials_list_routing
npm install
npm run build
npm start
Notes: