Skip to content
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 travelTime Data Functionality #376

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

CasperL1218
Copy link
Contributor

@CasperL1218 CasperL1218 commented Oct 27, 2024

Summary

This pull request implements travel time data functionality for apartment pages, providing users with distance information from apartments to major campus locations.

Key implementations:

  • Created new travelTimes collection and corresponding data types
  • Implemented Google Maps Matrix API integration for travel time calculations
  • Added backend endpoints for travel time data retrieval
  • Enhanced frontend map functionality with travel time display
  • Improved mobile map experience and map interaction behavior

Test Plan

Backend Testing:

  • Google Maps Matrix API integration validation
  • Single building travel time document creation
travelTime single building api postman travelTime single doc db
  • Full travel time collection initialization
  • Backend endpoint functionality for data retrieval
travelTime batch doc initialization travelTime batch doc initialization 2 db travelTimes collection

Frontend Testing:

  • Travel time data display in MapInfo component
map travelTimes data display
  • Travel time data display in MapModal component
map modal travelTime data display
  • Mobile map modal functionality
mobile map modal
  • Map auto-recentering behavior
map.auto-recenter.mov
  • School location icons alignment with travel time data points
map school icon count update

Notes

Database Schema Changes:

  • New collection: travelTimes
  • Schema structure:
    • Id: building id (matching building collection)
    • Fields:
      • walking_arts_quad
      • walking_duffield
      • walking_ctb
      • driving_arts_quad
      • driving_duffield
      • driving_ctb

API Integration:

  • Added Google Maps Matrix API integration
  • Note: API calls only required during new apartment addition

Frontend Enhancements:

  • Implemented mobile-responsive map modal
  • Added auto-recentering functionality for MapInfo component
  • Updated school location icons to match travel time data points

Breaking Changes

Database Updates:

  • Added new travelTimes collection
  • Created new LocationTravelTimes type in db-types
  • Modified database schema to include travel time data

Future Considerations

  • Update "add_buildings" script to automatically generate corresponding travelTimes documents
  • Optimize API usage for batch apartment additions
  • Consider caching strategies for frequently accessed travel time data

- Created travelTime collection
- Implemented POST endpoint to calculate travel distances using Google Map Matrix API
- Implemented testing POST endpoint to create single building document
@dti-github-bot
Copy link
Member

dti-github-bot commented Oct 27, 2024

[diff-counting] Significant lines: 531.

Copy link
Member

@kea-roy kea-roy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Early PR Review

Overall, this is a great start on this PR. For the code you have implemented so far, I have suggested some minor things you could do to make it better. I hope that the rest of your work on this PR will be of the same splendid quality :)

backend/src/app.ts Outdated Show resolved Hide resolved
backend/src/app.ts Outdated Show resolved Hide resolved
backend/src/app.ts Outdated Show resolved Hide resolved
- Implemented travel documents document batch creation endpoint
- Tested on smaller batches (size 10)
- Created building travel time documents for all buildings
- Created endpoint for frontend travel time retrieval
- Improved documentation for helper function
- Implemented frontend travel time display
- Mofified ApartmentWithId type to include travelTimes attribute
- Modified TravelTimes type (isolated from Apartments type)
- Implemented map recenter feature upon map modal closure
- Implemented mobile map modal
@CasperL1218 CasperL1218 changed the title WIP: Implemented travelTime Firebase Data Implemented travelTime Firebase Data Nov 16, 2024
@CasperL1218 CasperL1218 changed the title Implemented travelTime Firebase Data Implemented travelTime Data Functionality Nov 16, 2024
Copy link
Contributor

@ggsawatyanon ggsawatyanon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job on this map backend PR! The map feature is definitely nearing completion and the travel time info will be very useful for users. I'm requesting changes for you to implement the small frontend change that David had mentioned (increasing the space between the location name and the map on the mobile modal) as well as adjusting the minimum font size of the location names in the mobile ver. of map modal to match the figma (13px, right now it's 10px and quite difficult to read on mobile) . Once the spacing and font size is fixed, you'll be fully finished with the map feature!

- Added spacing between address and map
- Increased location and traveltime font size for better visibility
@CasperL1218
Copy link
Contributor Author

Great job on this map backend PR! The map feature is definitely nearing completion and the travel time info will be very useful for users. I'm requesting changes for you to implement the small frontend change that David had mentioned (increasing the space between the location name and the map on the mobile modal) as well as adjusting the minimum font size of the location names in the mobile ver. of map modal to match the figma (13px, right now it's 10px and quite difficult to read on mobile) . Once the spacing and font size is fixed, you'll be fully finished with the map feature!

Thanks for catching that! I have implemented the frontend improvement regarding the mobile map modal.
Screenshot 2024-11-17 at 01 08 28

Copy link
Member

@kea-roy kea-roy left a 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 an excellent PR. Your implementation is responsive and correctly adapts the figma design. Your API endpoints seem mostly correct, however I suggested a few minor changes just to make sure it works when the API is not run locally. There are a couple of minor things you can do to further improve your code, such as fixing some documentation, so I've also suggested those changes with my comments :) Overall, great PR!

@@ -960,4 +964,269 @@ app.post('/api/add-contact-question', authenticate, async (req, res) => {
}
});

interface TravelTimes {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you already imported LocationTravelTimes, you can delete TravelTimes and use the imported type.

}

// Calculate travel times using the main endpoint
const response = await axios.post(`http://localhost:3000/api/calculate-travel-times`, {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can delete http://localhost:3000 from the axios post request. Keeping this part in the request means it will only work if you run the backend locally, but not on prod. I believe replacing it with axios.post('/api/calculate-travel-times') may do the trick, but I didn't test this modification.

return;
}

const response = await axios.post(`http://localhost:3000/api/calculate-travel-times`, {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, you can delete http://localhost:3000 from the axios post request.

* Looks up the travel times document for the given building ID and returns the stored walking and driving
* times to Cornell landmarks: Engineering Quad, Agriculture Quad, and Ho Plaza.
*
* @route GET /api/travel-times/:buildingId
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adjust to /api/travel-times-by-id/:buildingId

@@ -150,6 +156,17 @@ const ApartmentPage = ({ user, setUser }: Props): ReactElement => {
const saved = savedIcon;
const unsaved = unsavedIcon;
const [isSaved, setIsSaved] = useState(false);
const mapInfoRef = useRef<MapInfoRef>(null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember to clean up unused variables! Just look at the check warnings from lint to see what is unused.

const Modals = landlordData && apt && (
<>
<MapModal
aptName={apt!.name}
open={mapOpen}
onClose={() => setMapOpen(false)}
onClose={handleMapModalClose}
setOpen={setMapOpen}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your setOpen parameter is no longer being used, so maybe remove setOpen as a parameter?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants