-
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
Implemented Google Map (New Version) in Apartment Page #357
Conversation
[diff-counting] Significant lines: 728. |
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
This is a really nice implementation of the map. The front-end design looks great on large and small screens, adapting correctly to the page layout even for mobile devices. It looks nearly identical to the Figma design, so great work!
Even though the design is really good, I noticed a few things that could be improved.
- The pin in the Figma design also has a small red ellipse below the pin, but the SVG file you used doesn't include that ellipse. You could consider updating the SVG file used to include the ellipse or check with the designers if the red ellipse is needed.
- Remove console.log command in MapInfo.tsx: This prints the API key to the console. While good for testing, this is unnecessary and it is probably better to omit it from the console for security reasons.
[MapInfo.tsx]
40: console.log(process.env.GOOGLE_MAP_API_KEY);
- Improving the documentation of your code, for instance adding a doc comment for your MapInfo component, just to make the code easier to understand.
[MapInfo.tsx]
32: // add documentation here
33: export default function MapInfo({
Overall, these are minor changes and you did a great job!
- Update apartment icon pin image - Remove development console log message - Added component documentation for MapInfo component
- Implemented map modal component - Removed driving time info from default map - Added landmark distance info to location information - Added documentation for map modal component Future tasks - Implement location info for modal map (landmarks, driving/waling time) - Implement recenter button functionality - Decrease marker size for apartment/landmarks to fit design
- Implemented recenter button - Updated icon for campus landmarks - Updated distance information layout for map modal
- Implemented zoom in/out buttons for small map and map modal - Resolved firebase import issues by adding new export and import statements
The implementation of the zoom functionality update tackled 2 issues:
|
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
Hi Casper, your implementation of the zoom in-out buttons is going great, I've tested the look and the zoom button's responsiveness on different screens and you have made sure it adapts well. Good work on that!
However, I noticed a few responsiveness issues that could still be addressed.
- Main screen: wrap text issue
- Although this is pretty minor, certain screen sizes cause the text to wrap around, creating an inconsistent design. Maybe check with the designers on what the design should look like if the text doesn't fit: most likely either abbreviating the text with ellipsis or making all of the text wrap around and slightly indented.
- Modal responsiveness: Information cut off
- Even though the map itself is very responsive, the dialog containing all the information is not quite there yet. If a scrollable element is not recommended by the designers, consider removing all of the text below the map on smaller screens (check with designers on this for feedback).
- Zoom in and out icons
Remember to update the icons with the correct ones when you get the designs from the designers!
Overall, these are small design issues that just require a bit more specification and feedback from the team. The actual functionality is great! So good job on this PR!
Summary
This pull request is the first step toward implementing the Google Map & distance from campus information for the Apartment page.
Google Map:
Location information:
Test Plan
Location section and Google Map
Google Map drag and move
Google Map zoom in
Google Map zoom out
Google map full-screen expansion
Google Map drag and move, zoom in/out (full-screen)
Notes
*Note: the time required for walking/driving to campus is currently hardcoded to 0
Future Plans