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

Feature/add contributions to map #742

Merged
merged 18 commits into from
Sep 25, 2020
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
# Changelog
## [Unreleased]
### Enhancements
* Add map to contributions page that dynamically adds pins for Asks/Offers

### Breaking changes
* Users can now register themselves (was previously blocked by #660)

Expand Down
Binary file added app/assets/images/ask.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added app/assets/images/offer.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
1 change: 1 addition & 0 deletions app/assets/stylesheets/application.scss
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
@import './listings'; // TODO: remove this once listings/new is refactored

@import './mapbox-gl';
@import './maps';
Copy link
Collaborator

Choose a reason for hiding this comment

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

@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


// TODO: use has-text-centered instead
.text-center {
Expand Down
30 changes: 30 additions & 0 deletions app/assets/stylesheets/maps.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
#map {
width: 100%;
height: 500px;
}

.marker {
background-size: cover;
width: 50px;
height: 50px;
border-radius: 50%;
cursor: pointer;
}

.ask-marker {
background-image: url('~./../images/ask.png');
}

.offer-marker {
background-image: url('~./../images/offer.png');
}

.mapboxgl-popup {
max-width: 200px;
}

.mapboxgl-popup-content {
text-align: center;
font-family: 'Open Sans', sans-serif;
h3 { font-weight: bold; }
}
5 changes: 3 additions & 2 deletions app/blueprints/contribution_blueprint.rb
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
class ContributionBlueprint < Blueprinter::Base
identifier :id
association :categories_for_tags, name: :category_tags, blueprint: DefaultBlueprint
association :service_area, blueprint: DefaultBlueprint
association :service_area, blueprint: ServiceAreaBlueprint, view: :with_location
association :contact_types, blueprint: DefaultBlueprint do |contribution, _options|
[contribution.person.preferred_contact_method]
end
association :urgency, blueprint: DefaultBlueprint do |contribution, _options|
UrgencyLevel.find(contribution.urgency_level_id)
end
fields :title, :description, :inexhaustible
association :location, blueprint: LocationBlueprint
fields :title, :description, :inexhaustible, :name
field :created_at do |contribution, _options|
contribution.created_at.to_f * 1000 # Javascript wants miliseconds, not seconds
end
Expand Down
2 changes: 1 addition & 1 deletion app/blueprints/service_area_blueprint.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
class ServiceAreaBlueprint < Blueprinter::Base
identifier :id

fields :name, :description
fields :name, :description, :location
Copy link
Collaborator

Choose a reason for hiding this comment

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

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).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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


view :with_location do
association :location, blueprint: LocationBlueprint
Expand Down
4 changes: 3 additions & 1 deletion app/javascript/pages/Browse.vue
Original file line number Diff line number Diff line change
Expand Up @@ -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"/>
Copy link
Collaborator

@indiebrain indiebrain Sep 25, 2020

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@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

Copy link
Collaborator

Choose a reason for hiding this comment

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

@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.

</section>
</div>
</template>
Expand All @@ -28,6 +28,7 @@ import BrowserSelector from './browse/BrowserSelector'
import Filters from './browse/Filters'
import ListBrowser from './browse/ListBrowser'
import TileBrowser from './browse/TileBrowser'
import MapBrowser from './browse/MapBrowser'
import ContributionFetcher from './browse/ContributionFetcher'

export default {
Expand All @@ -36,6 +37,7 @@ export default {
Filters,
ListBrowser,
TileBrowser,
MapBrowser,
},
props: {
contributions: {type: Array},
Expand Down
208 changes: 117 additions & 91 deletions app/javascript/pages/browse/MapBrowser.vue
Original file line number Diff line number Diff line change
Expand Up @@ -2,105 +2,131 @@
<section class="MapBrowser">
<!-- FIXME Pull access-token via environment variable -->
<mapbox
access-token="pk.eyJ1IjoibXV0dWFsLWFpZC1hcHAiLCJhIjoiY2tmZTBvd3UwMDBhbTJ4cDlic2JmMWZoaiJ9.rWscBjdl1SMT5N0yekIJYg"
:map-options="{
style: 'mapbox://styles/mapbox/streets-v11',
center: [-75.1635262, 39.9527237],
zoom: 9,
}"
:geolocate-control="{
show: true,
position: 'top-left',
}"
@map-load="loaded"
@map-zoomend="zoomend"
@map-click:points="clicked"
@geolocate-error="geolocateError"
@geolocate-geolocate="geolocate"
/>
:access-token= this.accessToken
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

:map-options="{
style: 'mapbox://styles/mapbox/streets-v11',
center: [-96, 37.8],
zoom: 4,
}"
:geolocate-control="{
show: true,
position: 'top-left',
}"
@map-load="loaded"
@map-zoomend="zoomend"
@map-click:points="clicked"
@geolocate-error="geolocateError"
@geolocate-geolocate="geolocate"
/>
</section>
</template>

<script>
import Mapbox from 'mapbox-gl-vue'
import Mapbox from 'mapbox-gl-vue'
const mbxClient = require('@mapbox/mapbox-sdk');
const geoCoding = require('@mapbox/mapbox-sdk/services/geocoding');
// FIXME Attempt to pass accessToken in script tag as seen in line 6
const baseClient = mbxClient({ accessToken: "pk.eyJ1IjoibXV0dWFsLWFpZC1hcHAiLCJhIjoiY2tmZTBvd3UwMDBhbTJ4cDlic2JmMWZoaiJ9.rWscBjdl1SMT5N0yekIJYg"});
const geoCodingService = geoCoding(baseClient)

export default {
components: { Mapbox },
methods: {
loaded(map) {
var geojson = {
'type': 'FeatureCollection',
'features': [
{
'type': 'Feature',
'properties': {
'message': 'Foo',
'icon': 'harbor',
'iconSize': [60, 60]
},
'geometry': {
'type': 'Point',
'coordinates': [-75.1635262, 39.9527237]
}
},
]
}
export default {
props: {
accessToken: String,
contributions: {type: Array, default: () => []},
filters: Object,
},
components: { Mapbox },
methods: {
loaded(map) {
Copy link
Collaborator

@solebared solebared Sep 25, 2020

Choose a reason for hiding this comment

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

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 :).

Copy link
Collaborator Author

@phylum phylum Sep 25, 2020

Choose a reason for hiding this comment

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

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.

let geojson = {
'type': 'FeatureCollection',
'features': []
}

map.addLayer({
id: 'points',
type: 'symbol',
source: {
type: 'geojson',
data: geojson,
},
layout: {
'icon-image': '{icon}-15',
'text-field': '{title}',
'text-font': ['Open Sans Semibold', 'Arial Unicode MS Bold'],
'text-offset': [0, 0.6],
'text-anchor': 'top',
},
})
const buildGeoJson = () => {
return new Promise((resolve) => {
let forwarded = 0

geojson.features.forEach(function (marker) {
var el = document.createElement('div');
el.className = 'marker';
el.style.backgroundImage =
'url(https://placekitten.com/g/5/5/)';
el.style.width = marker.properties.iconSize[0] + 'px';
el.style.height = marker.properties.iconSize[1] + 'px';
this.contributions.forEach(contribution => {
geoCodingService.forwardGeocode({
query: `${contribution.location.street_address} ${contribution.service_area.location.city} ${contribution.service_area.location.state} ${contribution.service_area.location.zip_code}`,
limit: 1
}).send()
.then(response => {
++forwarded

el.addEventListener('click', function () {
window.alert(marker.properties.message);
});
const match = response.body;

new mapboxgl.Marker(el)
.setLngLat(marker.geometry.coordinates)
.addTo(map);
});
},
zoomend(map, e) {
console.log('Map zoomed')
},
clicked(map, e) {
const title = e.features[0].properties.title
console.log(title)
},
geolocateError(control, positionError) {
console.log(positionError)
},
geolocate(control, position) {
console.log(
`User position: ${position.coords.latitude}, ${position.coords.longitude}`
)
},
},
}
</script>
geojson.features = [ ...geojson.features, {
'type': 'Feature',
'properties': {
'title': contribution.name,
'description': contribution.description ? contribution.description : "",
'categoryTag': contribution.category_tags[0].name,
'urgency': contribution.urgency ? contribution.urgency : "",
'icon': 'harbor',
'iconSize': [50, 50],
'contributionType': contribution.contribution_type,
},
'geometry': {
'type': 'Point',
'coordinates': match.features[0].center
}
} ]
})
.then(() => {
if (forwarded === this.contributions.length) resolve()
})
})
})
}

buildGeoJson()
.then(() => {
map.addLayer({
id: 'points',
type: 'symbol',
source: {
type: 'geojson',
data: geojson,
},
layout: {
'text-font': ['Open Sans Semibold', 'Arial Unicode MS Bold'],
'text-offset': [0, 0.6],
'text-anchor': 'top',
},
})

geojson.features.forEach(function (marker) {
var el = document.createElement('div');
el.className = 'marker ' + marker.properties.contributionType.toLowerCase() + '-marker';
el.style.width = marker.properties.iconSize[0] + 'px';
el.style.height = marker.properties.iconSize[1] + 'px';

<style>
#map {
width: 100%;
height: 500px;
const newMarker = new mapboxgl.Marker(el)
.setLngLat(marker.geometry.coordinates)
.setPopup(new mapboxgl.Popup({ offset: 35 })
.setHTML('<h3>' + marker.properties.title + '</h3><p>' + marker.properties.categoryTag + '</p><p>' + marker.properties.description + '</p>'))

newMarker.addTo(map)
})
})
},
zoomend(map, e) {
console.log('Map zoomed')
},
clicked(map, e) {
const title = e.features[0].properties.title
console.log(title)
},
geolocateError(control, positionError) {
console.log(positionError)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

},
geolocate(control, position) {
console.log(
`User position: ${position.coords.latitude}, ${position.coords.longitude}`
)
},
},
}
</style>
</script>
1 change: 1 addition & 0 deletions app/models/system_setting.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these supposed to be checked in?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the clarification @exbinary !

# donations_module :boolean default(TRUE), not null
# exchange_type :string default("peer_to_peer"), not null
# landing_page_text_how :text
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
},
"dependencies": {
"@fortawesome/fontawesome-free": "^5.13.0",
"@mapbox/mapbox-sdk": "^0.11.0",
"@rails/ujs": "^6.0.0",
"@rails/webpacker": "4.2.2",
"@vue/composition-api": "^0.5.0",
Expand Down
1 change: 1 addition & 0 deletions spec/factories/system_settings.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Check this in or no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

# donations_module :boolean default(TRUE), not null
# exchange_type :string default("peer_to_peer"), not null
# landing_page_text_how :text
Expand Down
Loading