openplans / shareabouts

Shareabouts is a mapping application for crowdsourced info gathering.
GNU General Public License v3.0
276 stars 150 forks source link

Remove conditional loading of Mapbox GL on MAPBOX_TOKEN #229

Closed BenSturmfels closed 8 months ago

BenSturmfels commented 1 year ago

MAPBOX_TOKEN is only used for geocoding, not displaying maps, so there's no reason to use it to conditionally load Mapbox GL. It's also confusing when your map doesn't show up because there's no MAPBOX_TOKEN in the Django settings.

mjumbewu commented 10 months ago

The MAPBOX_TOKEN is also passed into JavaScript and used when creating the L.mapboxGL layer. But you're right, there are other ways to supply the access token (e.g. in the config file), so it can be confusing.

The intention there is to only load the mapbox-gl libraries if they're needed. They'll only be needed if you're loading map tiles from Mapbox. It might be better to check the config to see whether any of the config.map.layers have a type of 'mapbox'. But that might require some Django filter version of pluck, which on first glance, doesn't exist. E.g.:

{% if "mapbox" in config.map.layers|pluck:"type" %}
  <link href="...

Am I overcomplicating things?

BenSturmfels commented 10 months ago

I've just pushed an updated version that loads MapboxGL conditionally based on the presence of a mapbox layer type. @mjumbewu this is essentially what you suggested above, but my thinking was that it's better to keep the template simple by doing the config lookup in Python.

mjumbewu commented 8 months ago

I like it. I'm going to merge it in.