Feature/add contributions to map #742

phylum · 2020-09-25T15:14:59Z

Why

To allow contributions with locations to be plotted on the map view dynamically on the contributions page so that users can see the Asks/Offers closest to them.

References 708

Pre-Merge Checklist

All these boxes should be checked off before any pull request is merged!

All new features have been described in the pull request
Security & accessibility have been considered
All outstanding questions and concerns have been resolved
Any next steps that seem like a good ideas have been created as issues for future discussion & implementation
High quality tests have been added, or an explanation has been given why the features cannot be tested
New features have been documented, and the code is understandable and well commented
Entry added to CHANGELOG.md if appropriate

How

Here's a list of APIs used for the Maps:
mapbox-gl-vue
mapbox-sdk-js

Next Steps

Build a more complete location data for Asks/Offers to include City, State, Zipcode, if privacy concerns allow
Implement clustering so that as we zoom in/out we can see how many Asks/Offers are available
Dynamically centering on the top-level Service Area

Outstanding Questions, Concerns and Other Notes

Security

FIXME for Mapbox Access Token should be addressed to pull it from an ENV variable
Location data mentioned above in Next Steps

- added ask and offer image - move styles to its own maps stylesheet - updated geojson properties needed for pin rendering

- added descriptions of properties that can be pulled from contributions.json - removed text-field from layout: the pin would suffice - add styles for pin popup

…good/mutual-aid into feature/add-contributions-to-map

maebeale

LGTM, folks! WOO HOOOO!!!!! SUCH a huge addition to this app. Thank you!!!

indiebrain · 2020-09-25T15:30:34Z

app/javascript/pages/Browse.vue

@@ -18,7 +18,7 @@
      <section>
        <BrowserSelector :browser="browser" @clicked="browser = $event" />
      </section>
-      <component :is="browser" :contributions="activeContributions" class="row" />
+      <component :is="browser" :contributions="activeContributions" class="row" accessToken="pk.eyJ1IjoibXV0dWFsLWFpZC1hcHAiLCJhIjoiY2tmZTBvd3UwMDBhbTJ4cDlic2JmMWZoaiJ9.rWscBjdl1SMT5N0yekIJYg"/>


A series of questions:

Is accesToken a credential to the maps API?

Is this a sensitive credential? Does this get served down to the client? Is there any amount of armoring necessary to protect it?

EDIT: Next time, I shall read the full PR description first...

FIXME for Mapbox Access Token should be addressed to pull it from an ENV variable

This also seems to be copy-pasta'd in pp/javascript/pages/browse/MapBrowser.vue. Channeling Dave Thomas and Andy Hunt: "There should exist a single authoritative source of knowledge within a system - IE Do not repeat yourself."

Had you considered the case where we need to change the Map API credential? It seems likely that we might miss one or more instances with the current design.

@indiebrain yup, i believe we agreed to go with this path for this week, but that there's a ticket to move this out asap

@indiebrain I agree that this should be DRYed up a bit. Given the time we've spent thus far and familiarity with given clientside tech, I think everyone did a fantastic job!

All things are possible with enough time and money.. whiskey helps.

maebeale · 2020-09-25T15:36:59Z

here's the circleci error

…utions-to-map

thestephenmarshall

Looks pretty awesome so far. Thanks for all the hard work! There are a couple of comments to address. 🎸

thestephenmarshall · 2020-09-25T15:55:50Z

app/assets/stylesheets/application.scss

@@ -8,6 +8,7 @@
 @import './listings';  // TODO: remove this once listings/new is refactored

 @import './mapbox-gl';
+@import './maps';


@maebeale as time goes on, we will want to consider splitting packs up and this would likely be either: a) it's own pack or b) imported on a more view-specific pack. /randomthoughts

thestephenmarshall · 2020-09-25T15:58:45Z

app/javascript/pages/Browse.vue

@@ -18,7 +18,7 @@
      <section>
        <BrowserSelector :browser="browser" @clicked="browser = $event" />
      </section>
-      <component :is="browser" :contributions="activeContributions" class="row" />
+      <component :is="browser" :contributions="activeContributions" class="row" accessToken="pk.eyJ1IjoibXV0dWFsLWFpZC1hcHAiLCJhIjoiY2tmZTBvd3UwMDBhbTJ4cDlic2JmMWZoaiJ9.rWscBjdl1SMT5N0yekIJYg"/>


@indiebrain I agree that this should be DRYed up a bit. Given the time we've spent thus far and familiarity with given clientside tech, I think everyone did a fantastic job!

All things are possible with enough time and money.. whiskey helps.

thestephenmarshall · 2020-09-25T15:59:16Z

app/javascript/pages/browse/MapBrowser.vue

-        @geolocate-error="geolocateError"
-        @geolocate-geolocate="geolocate"
-      />
+      :access-token= this.accessToken


Nitpick: i don't think this. is necessary here.

thestephenmarshall · 2020-09-25T16:00:11Z

app/javascript/pages/browse/MapBrowser.vue

+        console.log(title)
+      },
+      geolocateError(control, positionError) {
+        console.log(positionError)


I think we might want to remove these console.log statements, but I will defer to what MA desires here.

thestephenmarshall · 2020-09-25T16:00:25Z

app/models/system_setting.rb

@@ -69,6 +69,7 @@ def peer_to_peer?
 #  confirmation_page_text_footer      :string
 #  confirmation_page_text_header      :string
 #  confirmation_page_text_link_header :string
+#  display_navbar                     :boolean          default(FALSE)


Are these supposed to be checked in?

Yes, we have a gem that automatically inserts these comments and keeps them up to date with new migrations.

Thanks for the clarification @exbinary !

thestephenmarshall · 2020-09-25T16:00:40Z

spec/factories/system_settings.rb

@@ -17,6 +17,7 @@
 #  confirmation_page_text_footer      :string
 #  confirmation_page_text_header      :string
 #  confirmation_page_text_link_header :string
+#  display_navbar                     :boolean          default(FALSE)


Check this in or no?

@thestephenmarshall Looks like this was inserted automatically. We didn't add the change manually so we should probably defer to @maebeale @exbinary @h-m-m for this.

Annotate adds these schema details for both models and factories after migrations are applied. I think it's okay to check this in unless mutual-aid folks say otherwise.

solebared

Looks good to me! TBH i can't provide a meaningful review of the mapbox related code but the general shape of things look good (a couple of very minor notes inline).

solebared · 2020-09-25T15:51:47Z

app/blueprints/service_area_blueprint.rb

@@ -1,7 +1,7 @@
 class ServiceAreaBlueprint < Blueprinter::Base
  identifier :id

-  fields :name, :description
+  fields :name, :description, :location


We probably don't want location to be a default field since its also available via the :with_location view (see 'Views' section in docs here).

Thanks, that was a minor oversight by us while we were learning our way around Blueprint

solebared · 2020-09-25T16:14:21Z

app/javascript/pages/browse/MapBrowser.vue

-        @geolocate-error="geolocateError"
-        @geolocate-geolocate="geolocate"
-      />
+      :access-token= this.accessToken


Nitpick: i don't think this. is necessary here.

solebared · 2020-09-25T16:22:24Z

app/javascript/pages/browse/MapBrowser.vue

+    },
+    components: { Mapbox },
+    methods: {
+      loaded(map) {


This looks great. On a second pass in the future, it could be worth trying to extract some of the sdk functions out into a separate module. There's a bit going on in this loaded method :).

this. is necessary because we are accessing MapBrowse's props and {{ accessToken }} didn't pass the token at all. Given more time to explore Vue.js we might be able to improve on this, however.

solebared · 2020-09-25T16:23:18Z

app/models/system_setting.rb

@@ -69,6 +69,7 @@ def peer_to_peer?
 #  confirmation_page_text_footer      :string
 #  confirmation_page_text_header      :string
 #  confirmation_page_text_link_header :string
+#  display_navbar                     :boolean          default(FALSE)


Yes, we have a gem that automatically inserts these comments and keeps them up to date with new migrations.

phylum and others added 14 commits September 23, 2020 15:03

Provide contributions to MapBrowser

63d1b23

Wip - work on getting access to service area location

b12c8a1

Render pins according to contribution type

af4fc35

- added ask and offer image - move styles to its own maps stylesheet - updated geojson properties needed for pin rendering

Adding popups on click

b7765fd

- added descriptions of properties that can be pulled from contributions.json - removed text-field from layout: the pin would suffice - add styles for pin popup

WIP: Add contributions to map

e4f0c6c

Cleanup contributions view

263874b

WIP: Add contributions to map

e406b3e

Merge branch 'feature/add-contributions-to-map' of github.com:rubyfor…

a325c53

…good/mutual-aid into feature/add-contributions-to-map

Display contributions on map

a906762

Update spec

96008f9

Make table is-fullwidth

3efb600

Refactor code and reposition map center

52424e9

Merge main, fix conflicts and increase zoom on map

355b694

Update Changelog

531fd80

phylum requested review from solebared, thestephenmarshall, h-m-m and maebeale September 25, 2020 15:14

phylum assigned maricris-sn, thestephenmarshall, phylum and danielmmack Sep 25, 2020

maebeale approved these changes Sep 25, 2020

View reviewed changes

indiebrain reviewed Sep 25, 2020

View reviewed changes

phylum added 3 commits September 25, 2020 11:57

Update contribution blueprint rspec

c507216

Merge branch 'main' into feature/contributions-cleanup

70662d8

Merge branch 'feature/contributions-cleanup' into feature/add-contrib…

376b8e3

…utions-to-map

thestephenmarshall reviewed Sep 25, 2020

View reviewed changes

solebared approved these changes Sep 25, 2020

View reviewed changes

Passing ruby tests

f124060

phylum merged commit cf9093d into main Sep 25, 2020

phylum deleted the feature/add-contributions-to-map branch September 25, 2020 17:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/add contributions to map #742

Feature/add contributions to map #742

phylum commented Sep 25, 2020 •

edited by danielmmack

Loading

maebeale left a comment

indiebrain Sep 25, 2020 •

edited

Loading

indiebrain Sep 25, 2020

maebeale Sep 25, 2020

thestephenmarshall Sep 25, 2020

maebeale commented Sep 25, 2020

thestephenmarshall left a comment

thestephenmarshall Sep 25, 2020

thestephenmarshall Sep 25, 2020

thestephenmarshall Sep 25, 2020

solebared Sep 25, 2020

thestephenmarshall Sep 25, 2020

thestephenmarshall Sep 25, 2020

solebared Sep 25, 2020

danielmmack Sep 25, 2020

thestephenmarshall Sep 25, 2020

phylum Sep 25, 2020

acherukuri Sep 25, 2020

solebared left a comment

solebared Sep 25, 2020

phylum Sep 25, 2020

solebared Sep 25, 2020

solebared Sep 25, 2020 •

edited

Loading

phylum Sep 25, 2020 •

edited

Loading

solebared Sep 25, 2020

Feature/add contributions to map #742

Feature/add contributions to map #742

Conversation

phylum commented Sep 25, 2020 • edited by danielmmack Loading

Why

Pre-Merge Checklist

How

Next Steps

Outstanding Questions, Concerns and Other Notes

Security

maebeale left a comment

Choose a reason for hiding this comment

indiebrain Sep 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maebeale commented Sep 25, 2020

thestephenmarshall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

solebared left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

solebared Sep 25, 2020 • edited Loading

Choose a reason for hiding this comment

phylum Sep 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phylum commented Sep 25, 2020 •

edited by danielmmack

Loading

indiebrain Sep 25, 2020 •

edited

Loading

solebared Sep 25, 2020 •

edited

Loading

phylum Sep 25, 2020 •

edited

Loading