add support for GeoJSON attribution #6364
Conversation
andrewharvey
force-pushed
the
geojson-attribution
branch
from
3240253 to
b55c935
Compare
andrewharvey
force-pushed
the
geojson-attribution
branch
from
b55c935 to
cc1a363
Compare
There was a problem hiding this comment.
This implementation looks good to me. Thanks for adding tests 😸
@jfirebaugh seems like you were on board with adding this functionality in #1485 – does this work for you?
| @@ -310,6 +310,10 @@ | |||
| "default": 18, | |||
| "doc": "Maximum zoom level at which to create vector tiles (higher means greater detail at high zoom levels)." | |||
| }, | |||
| "attribution": { | |||
| "type": "string", | |||
| "doc": "Contains an attribution to be displayed when the map is shown to a user." | |||
There was a problem hiding this comment.
I think it would be a good idea to explain here that this is added specifically to the AttributionControl
There was a problem hiding this comment.
The style-spec is platform independent so I'd say best not to refer to the platform specific AttributionControl. What do you think?
That said, it should be consistent with the other sources that accept an attribution option like vector, raster and raster-dem sources. eg. https://www.mapbox.com/mapbox-gl-js/style-spec/#sources-vector-attribution so any change in this wording should apply to all the others too.
There was a problem hiding this comment.
ah good point. 👍 to keep as is
| @@ -110,6 +111,7 @@ class GeoJSONSource extends Evented implements Source { | |||
|
|
|||
| if (options.maxzoom !== undefined) this.maxzoom = options.maxzoom; | |||
| if (options.type) this.type = options.type; | |||
| if (options.attribution) this.attribution = options.attribution; | |||
There was a problem hiding this comment.
should we validate that options.attribution is a non-empty string?
There was a problem hiding this comment.
The attribution option is optional so is that needed? If the attribution option isn't provided, or if it's an empty string it won't set any attribution on the source and the AttributionControl won't add any attribution for it.
There was a problem hiding this comment.
sorry I was unclear– I was thinking of validating that it is a string (as opposed to another type) and that its "non-empty." I was thinking about just white space, not a true nonempty which would be truthy). the first part is already happening in the style validation code which I forgot, and I'm not sure its worth worrying about the white space case after all because we don't do that for any other style spec values. sorry for confusion!
There was a problem hiding this comment.
That's right the style validation already spews out an error for a non-string value (handy if someone uses something like true by mistake. (Thanks for pointing that out by the way, I didn't actually realise it did that already)
As for white space, I'd say we allow it, if someone wants an attribution of "" or " " we should just comply.
|
@mollymerp not sure if this slipped through the cracks, or it needed further changes? Thanks! |
mollymerp
added
cross-platform 📺
Launch Checklist
adds support for custom attribution strings in GeoJSON sources. This implements the latter part of the discussion of #1485, but could be used as a workaround for the original issue by using a dummy GeoJSON source.
I think to close #1485 we would still need to allow custom attribution strings passed directly to the AttributionControl not attached to any source.
The Vector and Raster sources set their attribution properties via the shared TileJSON code, but since GeoJSON is loaded directly without a TileJSON, they need to be handled separately in the GeoJSON Source code.