openoakland / woeip

A platform for impacted communities to understand their local air quality and advocate for environmental justice.
https://woaq.org
MIT License
29 stars 16 forks source link

Kh/style data points #364

Closed kbrdsk closed 2 years ago

kbrdsk commented 3 years ago

Checklist

Description

Issue: #237

Renders pollution data points as a geojson data layer in mapbox instead of individual markers - should improve speed a bit when moving around the map. Pulls styling for data layer from the mapbox api by referencing a prefix on relevant layers (data-driven at the moment).

netlify[bot] commented 3 years ago

✔️ Deploy Preview for woeip ready!

🔨 Explore the source changes: ce357ef0990f8a7d87752f8675aa8241b320ebe9

🔍 Inspect the deploy log: https://app.netlify.com/sites/woeip/deploys/614b08ce288aee0008006771

😎 Browse the preview: https://deploy-preview-364--woeip.netlify.app/

kbrdsk commented 3 years ago

@kwonangela7 here's the pr draft I made, I haven't been able to get it running as a merge on the latest pull yet, list of things to do is

  1. get that working
  2. update .env variables to match production mapbox account
kwonangela7 commented 3 years ago

Thanks @kbrdsk! I have a few questions:

  1. Are you able to complete both 1 & 2, and if so, when? If not, should we set up a session with another developer who can take over the rest of the work that needs to be done for this PR?
  2. When you say "I haven't been able to get it running as a merge on the latest pull yet", what do you mean? I interpreted this as you haven't fixed the inconsistencies in your code that were brought to light when you pulled the latest code + I can see that the Github Actions checks are failing, but I'm not sure if I was interpreting this correctly.
  3. I know you mentioned over Slack that this was an incomplete PR. Could you further define that? Will it be completed once you do 1 and 2 mentioned in your comment above?
kbrdsk commented 3 years ago
  1. I'll take a look at but I'm not sure how long it'll take me. When I stepped back in I was having trouble getting the latest version of dev running properly (I had some errors with uploading files etc) without including changes from this PR. I was hoping that if someone else had it running properly that this wouldn't introduce any breaking changes. But once this is up and running and the .env variables have been set all you should need to do is change things in mapbox studio.

  2. I can do I'd just need to info from the production database - it's probably better for me to write documentation for it somewhere though. Right now there are 3 environment variables that need to be changed.

image

The first two, REACT_APP_MAPBOX_TOKEN and REACT_APP_MAPBOX_STYLE_URL are needed to connect to the mapbox api and can be pulled from the mapbox account. Docs for the token are here, you'll want to go to https://account.mapbox.com/access-tokens/ in the production account. The mapbox style url you get once you've actually created a style in mapbox studio (docs here).

The last variable REACT_APP_MAPBOX_SESSION_STYLE_PREFIX you should put on the id of any layer you create in studio to style the pollutant data and can be whatever you think is best.

Screenshot 2021-09-14 21 00 33

In order to create the styles you can upload some sample GeoJson data to mapbox studio and style that so you can see how it looks, then attach the prefix to the names of the layers you create and the code will know to apply them to the actual data in the app.

sample-data.zip

All 3 variables should be set in both .env.development and .env.production, (right now I just have them set to Brooks' setup in .env.production).

Also all of this is only if @kwonangela7, @TangoYankee, and @theecrit think it even makes sense to have the workflow this way. I've spent way more time than I should have going back and forth on it and don't have particularly strong feelings either way. Changing the data layer to geojson seems important for the performance improvements on the map though.

kbrdsk commented 3 years ago

Oh and both @brooksjessup and I were wondering about how to translate the data to different AQI colors so maybe this is a good place to get confirmation - We should be using the pm2.5 levels corresponding to colors on pages 8 and 9 of the doc @kwonangela7 linked in the issue and the data we gather is in mg/m^3 so we just have to convert it to μg/m^3 to match the doc... is this correct?

theecrit commented 3 years ago

I wonder if @TangoYankee's latest improvements have fixed this issue?

When I stepped back in I was having trouble getting the latest version of dev running properly (I had some errors with uploading files etc) without including changes from this PR.

kwonangela7 commented 3 years ago

@kbrdsk re: coloring - yes that's correct!

re: # 1 - let's hope that Tim's fixes fix this, but if not, we can ask one of the devs to confirm that the latest changes are running fine, and see if they can test your PR out. The earliest we'll have someone on this is next Tuesday (and I'll post an update here if someone else is assigned to this), so just lmk if you figure it out before then.

re: # 2 Thank you for the explanation! I'm happy to do that once the PR is working.

Lastly, I like this workflow b/c it optimizes for more accessible development (it's easier to pick up mapbox than react).

kbrdsk commented 3 years ago

There was a typo in the code :sweat: but with that fixed and Tim's changes it looks like everything is working.

TangoYankee commented 3 years ago

There was a typo in the code 😓 but with that fixed and Tim's changes it looks like everything is working.

Oh, is this related to beta message? Then yes, that fix should be on develop. The other errors are related to handling network failures. I have a branch with those working changes. But, I am still writing their tests.

kbrdsk commented 3 years ago

There was a typo in the code 😓 but with that fixed and Tim's changes it looks like everything is working.

Oh, is this related to beta message? Then yes, that fix should be on develop. The other errors are related to handling network failures. I have a branch with those working changes. But, I am still writing their tests.

Sorry there was a typo in my code, not sure what the issues I was having with develop were earlier - those migh have been on my end as well

theecrit commented 3 years ago

@TangoYankee Is this merge-able at this stage?