Skip to content

Rebuild the signs work order map with mapbox#324

Open
chiaberry wants to merge 244 commits intostagingfrom
21517-app-next
Open

Rebuild the signs work order map with mapbox#324
chiaberry wants to merge 244 commits intostagingfrom
21517-app-next

Conversation

@chiaberry
Copy link
Copy Markdown
Member

Collector branch for cityofaustin/atd-data-tech#21517

I copied most of the map and the geocoder from the vision zero crash map. react-mapbox-gl is at version ^7.1.9 in vision zero, but the package say its on v8 now, so the imports are different and the types will be different.

cd signs-work-order-map

then to first spin up, nvm use and npm install and cp env_template .env, fill out the mapbox token.

npm run dev

You should see a full screen map. Test putting in an address in the upper left input. it should take you to that address
You can drag the map and the marker will recenter and i am console.logging the new lat/lon

For comparison, here is the existing map deployed https://atd-knack-signs-markings.netlify.app/

Does this feel like a good start to a collector branch?

@chiaberry
Copy link
Copy Markdown
Member Author

Something I completely overlooked when reviewing this repo last month was the calcs file, which renders some calculation widgets https://atd-knack-signs-markings.netlify.app/calcs, embedded in a page in knack. Christina is checking if this is still being used by SMD and if it needs to be included when we redeploy.

Copy link
Copy Markdown
Member

@johnclary johnclary left a comment

Choose a reason for hiding this comment

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

This looks so good! Thanks @chiaberry for getting us set up with scaffolding to rebuild this app 🚀 🚀 🚀

Comment thread signs-work-order-map/.nvmrc Outdated
Comment thread signs-work-order-map/app/globals.css
Comment thread signs-work-order-map/app/page.module.css
Comment thread signs-work-order-map/app/page.tsx
Comment thread signs-work-order-map/app/page.tsx
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

for consistency with VZE, should we save our vars to .env.local instead of .env? in earlier versions of nextjs i thought there were some important benefits from using .env.local vs .env for local dev, but as far as i can tell this is more of a consideration when using Vercel for deployments.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

i think the VZE consitency point is a good one. i think Next will use whichever one it finds so i think it is a matter of just renaming the file

Comment thread signs-work-order-map/package.json Outdated
@@ -0,0 +1,10 @@
{
"extends": ["next/core-web-vitals", "next/typescript", "prettier"],
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

when i run npm run lint i get an error about a missing prettier config 💥

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

i see this too. i think adding the eslint plugin like is in the VZE dev dependencies will get this back in business 😎

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I added the eslint plugins like vz. now im getting an error message when i run npm run lint
ESLint configuration in ../package.json » eslint-config-react-app is invalid:, I'll see where the mismatch is.

Comment thread signs-work-order-map/app/page.tsx Outdated
Copy link
Copy Markdown
Collaborator

@mddilley mddilley left a comment

Choose a reason for hiding this comment

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

Rad to have this map to start this rebuild 💪 And to have the geocoder in here already is so nice! I left some comments - this was a fun one to read and think through. I think this is a great start as a collector branch. 🗺️

@@ -0,0 +1,10 @@
{
"extends": ["next/core-web-vitals", "next/typescript", "prettier"],
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

i see this too. i think adding the eslint plugin like is in the VZE dev dependencies will get this back in business 😎

Comment thread signs-work-order-map/app/globals.css
Comment thread signs-work-order-map/app/layout.tsx Outdated
Comment thread signs-work-order-map/app/page.module.css
Comment thread signs-work-order-map/app/page.tsx
Comment thread signs-work-order-map/app/page.tsx Outdated
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

i think the VZE consitency point is a good one. i think Next will use whichever one it finds so i think it is a matter of just renaming the file

Copy link
Copy Markdown
Contributor

@mateoclarke mateoclarke left a comment

Choose a reason for hiding this comment

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

This looks good to me as a starting place/collector branch. 👍

paul-ryan-good-start

Comment thread signs-work-order-map/app/page.tsx Outdated
accessToken: MAPBOX_TOKEN || "",
bbox: ATX_BOUNDING_BOX,
});
// ctrl.on('loading', props.onLoading);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe this was just copy/pasta but will we need these commented lines?

});
// ctrl.on('loading', props.onLoading);
// ctrl.on('results', props.onResults);
ctrl.on("result", (evt) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I noticed above we call this param e, here we call it evt. My preference would be to consistency and verbosity with event

Comment thread signs-work-order-map/package.json Outdated
"version": "0.1.0",
"private": true,
"scripts": {
"dev": "next dev",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How would we feel about picking a non-default port number for this to run locally that doesn't conflict with VZE (:3002) or Moped Editor (:3000)?

For example:
"dev": "next dev -p 3003",

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I tried updating the package.json with this, but then I got 403s with the mapbox tiles. I dont understand why....

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I wonder if there's an IP whitelist for mapbox (i'm surprised that 3001 works with the VZE, though). there definitely is for Nearmap when it comes time to get that key set up

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Traditionally, these slippy-map keys have been restricted by what the referrer sent by the client when requesting resources is. To me, it's hand-wavy security, because it's pretty easy to write a proxy for them that you can use to mask the actual referrer sent from the browser, or to even just redefine the browser's own referrer behavior. That said, I can see that if you don't want to wade into the complexity of JWTs or similar, you gotta just ship that API key out to the client and hope for the best.

Anyway, Mapbox tokens are not required, but may include, these referrer restrictions. See the following screenshot from the token issuance page for some more info.

Screenshot 2025-04-21 at 11 26 33 AM

@johnclary
Copy link
Copy Markdown
Member

johnclary commented Apr 8, 2025

Updates look good! Should we hold off on approving, since this is a collector? Or can staging be the collector branch (since there's no CI?)?

@chiaberry
Copy link
Copy Markdown
Member Author

@johnclary I was thinking of not merging this branch, but you are right since there is no CI, we could use staging as the collector branch! Now I am not sure.

Adds pop-ups for rendered AGOL sign locations
…de ASSET_LOCATION_ID, show form on iframe load
…ocations

Adds ability to add existing locations
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.

7 participants