-
Notifications
You must be signed in to change notification settings - Fork 3
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
Allow Users to Save Landlord And Apartments #334
Conversation
[diff-counting] Significant lines: 370. |
Visit the preview URL for this PR (updated for commit 58c19a2): https://cu-apts-staging--pr334-save-apartment-landl-k9khkouq.web.app (expires Fri, 08 Dec 2023 21:58:07 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 096ac87b789b31770a01964fe0aaa92d563b9353 |
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.
Ankit, excellent work on the pull request! Your implementation to save landlords and apartments is both well-structured and efficiently designed. I appreciate the modular approach you took, especially in the backend with the creation of the "check-saved-apartment" and "check-saved-landlord" endpoints. This not only enhances code readability but also makes it easier to maintain and extend in the future. Your changes in the frontend, particularly in ApartmentCard, ApartmentPage, and LandlordPage, showcase a thoughtful integration of saved/unsaved icon buttons, providing users with a seamless experience. The attention to detail in handling user credentials in ApartmentCard and addressing related modifications in parent files demonstrates a keen understanding of the codebase. Your comprehensive test plan, covering both mobile and web changes, along with meticulous API testing and manual verification in Firebase, ensures the robustness of the functionality. Overall, great job on delivering a well-crafted and thoroughly tested solution!
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.
Hey Ankit! Great job on this pull request - incorporating the functionality to save landlord and apartments is a feature that will enhance the user experience with CUApts! The code looks good and great job on implementing both the backend and frontend of this task.
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.
Good job! In the future, we can consider converting user to a global context to prevent duplicating code of passing user to every component that we have.
Summary
This pull request allows users to save landlords and apartments. The implementation is spread out through several different parts.
Backend
Frontend
A problem I ran into is that in ApartmentCard, we need user credentials, however, they aren't passed through Props. So I had to update ApartmentCard and all parent files that used it or its parents to pass through the user from App.tsx. This means that some files that seemed unrelated were modified because they eventually called a parent of ApartmentCard like ApartmentCards.
Test Plan
Testing was done through mobile and web changes. API's were tested and followed almost identical format as other heavily tested endpoints. Additionally, all functionality was verified by manually looking into Firebase. A video below demonstrates the functionality:
p1.mov
p2.mov
Notes
None
Breaking Changes
None