Documentation: using addLayer() to insert layer before, the 'before' is unclear

[Spaxe]

mapbox-gl-js version: 1.0.0-beta6 documentation

In the example of https://www.mapbox.com/mapbox-gl-js/example/geojson-layer-in-stack/, it's not clear that "water" is a Layer or a Layer Group. I keep trying Layer Groups (e.g. "road", or "poi_label"), when in fact what I needed was something like "poi-scalerank4-l15".

The only reason I realised my mistake was because I found https://www.mapbox.com/blog/labels-on-top/ hours later.

An improvement might be a clearer message in the example code, as well as a warning in the console that the layer doesn't exist.

[andrewharvey]

Do you have a suggestion on how to improve?

To me it seems pretty clear "the addLayer method takes 2 arguments: the layer as an object, and a string representing another layer's name." and in the api doc "The ID of an existing layer to insert the new layer before. If this argument is omitted, the layer will be appended to the end of the layers array."

warning in the console that the layer doesn't exist.

I wonder if this should just be a warning or an error?

[Spaxe]

It's unfortunate that the id for both the example layer and layer group is water. Mapbox Studio also refers to these layer groups (for example, "road" under Mapbox Streets) as "layers", so the terminology won't cut it.

Here's a first try for improvement ...

    // method takes 2 arguments: the layer as an object, and a string
    // representing another layer's name. if the other layer
    // exists in the stylesheet already, the new layer will be positioned
    // right before that layer in the stack, making it possible to put
    // 'overlays' anywhere in the layer stack.

    // Note: layer group names like "landuse" will not work. Instead,
    // try individual layers in the group, such as "park", or "industrial".

[Spaxe]

I wonder if this should just be a warning or an error?

Mapbox's API design doesn't usually throw errors if an id is not found, so that would make it inconsistent. However, I would be in favour if at least it could throw a warning.

Printing an error would be ideal as long as code execution continues, that is, not a throw.

[andrewharvey]

It's unfortunate that the id for both the example layer and layer group is water. Mapbox Studio also refers to these layer groups (for example, "road" under Mapbox Streets) as "layers", so the terminology won't cut it.

The Mapbox Streets style doesn't have a "water" group so not sure where you're seeing that?

Layer groups are specific to Mapbox Studio, GL JS and the style spec don't mention groups so I'm not sure if it's a good idea to mix in concepts like layer groups, unless they are added to the style spec.

[Spaxe]

@andrewharvey The only way I could find out where the "water" comes from is here: https://www.mapbox.com/studio/styles/mapbox/streets-v9/ (near the bottom)

The terminology is an issue, but it's out of the scope of this issue. Second try...

    // method takes 2 arguments: the layer as an object, and a string
    // representing another layer's name. if the other layer
    // exists in the stylesheet already, the new layer will be positioned
    // right before that layer in the stack, making it possible to put
    // 'overlays' anywhere in the layer stack.
    // For more example see https://www.mapbox.com/blog/labels-on-top/

[andrewharvey]

https://www.mapbox.com/studio/styles/mapbox/streets-v9/

I see where you're coming from now, the "groups" in ^ are actually the source-layer which is from the Mapbox Streets v7 Tileset, not the Style Layer.

If you open it up in Studio eg. https://www.mapbox.com/studio/styles/mapbox/streets-v9/edit/ you'll get the layer id's listed on the left, or just download the Style JSON to inspect the layer's https://api.mapbox.com/styles/v1/mapbox/streets-v9?access_token=ACCESS_TOKEN

[andrewharvey]

ps #4225 discusses different solutions to make it easier to insert layers in a Style without relying on an arbitrary layer id.

[Spaxe]

Oh dear, the suggestions in #4225 are way more complicated than it needs to be. Thanks @andrewharvey. I'll take it from there.

[andrewharvey]

I had a go at implementing a warning, something like https://github.com/mapbox/mapbox-gl-js/compare/4690-warn-non-existant-before seems to work.

[Spaxe]

@andrewharvey That looks great. Can you enumerate the available layers to further avoid confusion?

e.g.

if (index < 0) {
   util.warnOnce("Layer '" + before + "' does not exist. Available layers are: " + this._order);
}