Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add Montreal data and page #57

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add Montreal data and page #57

wants to merge 2 commits into from

Conversation

beamalsky
Copy link
Collaborator

@beamalsky beamalsky commented Feb 13, 2025

As a means of getting reacquainted with this app, I've gone ahead and added a page for Montreal. Wow what a nice project to add to! This worked very smoothly so far.

To test:

  1. Import and process new Montreal data from OSM. This will take a while:

docker compose run --rm -w /app postgres make db/import/montreal.table

  1. Once that's completed, run the app: docker-compose up
  2. Visit the index page and make sure nothing is broken: http://localhost:8000/
  3. Visit the ✨ new Montreal page ✨ and check it out: http://localhost:8000/montreal

I'll leave some more notes below. This is my first time looking at a Django app in like 3 years so definitely open to code organization/style changes.

CleanShot 2025-02-12 at 18 52 47@2x

@@ -4,7 +4,7 @@ FROM python:3.8
# (see: https://github.com/nodesource/distributions/blob/master/README.md)
RUN curl -sL https://deb.nodesource.com/setup_12.x | bash -

RUN apt-get update && apt-get install -y --no-install-recommends nodejs gdal-bin
RUN apt-get update && apt-get install -y --no-install-recommends nodejs gdal-bin npm
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I got this error from docker compose build before explicitly installing npm here:

 > [app  9/11] RUN npm install:
0.192 /bin/sh: 1: npm: not found
------
failed to solve: process "/bin/sh -c npm install" did not complete successfully: exit code: 127

@@ -11,7 +11,7 @@ const canonicalComponents = [
const initAutocomplete = (inputElement, markerName, app) => {
// Create the autocomplete object
let autocomplete = new google.maps.places.Autocomplete(
inputElement, { componentRestrictions: { country: "us" } }
inputElement, { componentRestrictions: { country: ["us", "ca"], } }
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This means the autocomplete options will include both US and Canada results for both the Chicago and Montreal pages. I think this is fine because they're sorted by proximity to the user anyway, but if we wanted to we could only show US results on the Chicago page and Montreal results on the Canada page.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This thing:

CleanShot 2025-02-12 at 19 02 55@2x

</div>
<div class="form-group">
<div>
<input type="checkbox" id="enable-v2" name="enable-v2" checked />
Copy link
Collaborator Author

@beamalsky beamalsky Feb 13, 2025

Choose a reason for hiding this comment

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

This new file is mostly copied from index.html, other than turning on enable-v2 by default here and passing the city name to new App(). This could probably be DRYer but if we publish this let's talk through how the two pages would depart, if we actually want them at two different routes, etc.

try:
city = request.GET['city']
except KeyError:
city = 'chicago'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Chicago is hardcoded here as the city default if it's somehow not provided

@@ -139,14 +153,14 @@ def get_route(self, source_vertex_id, target_vertex_id, enable_v2=False):
WHEN mellow.type = ''route'' THEN way.reverse_cost * 0.75
ELSE way.reverse_cost
END AS reverse_cost
FROM chicago_ways AS way
FROM {city}_ways AS way
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was such a small adjustment needed to the SQL 🎉

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

Successfully merging this pull request may close these issues.

1 participant