-
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
Added sorting functionality to ApartmentCards component, Added sortApartments.ts file. #353
Added sorting functionality to ApartmentCards component, Added sortApartments.ts file. #353
Conversation
…ocation pages, and search page. Created a sortApartments method in sortApartments.ts
[diff-counting] Significant lines: 424. |
… Adjusted sorting options after feedback from designers and product managers. Simplified to a single drop down menu.
Commit Changes
|
Commit Changes
|
Commit Changes
|
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.
PR Review
Your implementation of the sortApartment.ts is extensive and complete, covering all the cases for sorting the apartments. The sorting system is robust and the display of the apartment list all look correct.
I believe these can be improved:
- Include some frontend UI photos in your PR/commits to better showcase the functionalities
- I noticed that there are some inconsistencies with the font styling with the dropdown menu.
- The "Sort by: " text should not have the colon
- The option texts should not be all capitalized
- The "Sort by" text and the text options of the dropdown should have a larger font
For comparison:
Overall these are minor issues and the implementation is great.
…wn component to avoid repetitive code. Fixed dropdown styling to better match figma
Thank you Casper for noticing those issues! I've created pull request #360 to resolve the issues you noticed and improve the code. |
…sx and moved svg into a separate file.
Created DropDownWithLabel component and Fixed dropdown styling to match Figma
… design on bookmarks page
Commit Changes
|
…Adjusted dropdown temporarily to be compatible with reviewmodal.
Commit Changes
|
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.
The PR is complete and comprehensive, especially with detailed documentation that make the code easy to follow. Just notice that there are some unused imports and fields in:
- frontend/src/components/Apartment/Header.tsx
- frontend/src/components/FAQ/CollapsibleQuestion.tsx
- frontend/src/components/Landlord/Header.tsx
- frontend/src/components/PhotoCarousel/PhotoCarousel.tsx
Overall it's a great job!
…components to better match current website scaling. Fixed ReviewModal custom dropdown styling to be left-aligned.
Commit Changes
|
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, Kea-Roy, this is a useful feature. Good documentation and styling. I also like the animation you added.
Summary
This pull request is the second step towards implementing improved sorting options on apartment card lists.
Test Plan
Tested on different screen sizes and mobile (for drop-down component look)
Tested sorting on different search results to ensure the relevance property sorts correctly.
Tested sorting on all fields with home page results, college town results, west results, and north results
Notes
The Dropdown menu item list has not been finalized and I'm including various items currently in this poll request to get feedback on which ones to keep/add.
The documentation for sortApartments.ts is also not completed. I've included commented-out code in case I need to use it after feedback.