V8 style support #39
V8 style support #39
Conversation
There was a problem hiding this comment.
@ryanhamley this looks solid! I think we should get the docs updated to clarify requirements for using this library:
- Streets v8
- Expression text values (no support for curly backets)
- Expects translated languages with coalesce expressions that fall back to non-null
namefield.
I see a few opportunities for improvement but I don't think these are necessary for merge:
- There is a somewhat common edge case where user will have a text field that isn't already wrapped in a coalesce (for example, if the style uses ["get", "name"] alone). Based on the logic implemented now, this could result in a lot of empty labels. To cover these cases, it'd be great if the plugin checked if field values are wrapped in coalesce, and if not, add the coalesce.
- To improve developer UX, add some validation to warn on curly bracket tokens, or warn on non-coalesce wrapped get expressions.
- There are rare edge cases where language get expressions are used in inputs to other expressions rather than outputs. Since the current implementation doesn't know about context when it applies transforms, there's no way to handle these cases. I would at least add a note about this in the readme. Take for example:
["match",
["get", "name"],
"Canada",
"America's Hat",
["coalesce",
["get", "name_en"],
["get", "name"]
]
]| @@ -5,7 +5,7 @@ | |||
| * @param {object} options - Options to configure the plugin. | |||
| * @param {string[]} [options.supportedLanguages] - List of supported languages | |||
| * @param {Function} [options.languageTransform] - Custom style transformation to apply | |||
| * @param {RegExp} [options.languageField=/^\{name/] - RegExp to match if a text-field is a language field | |||
| * @param {RegExp} [options.languageField=/^name_/] - RegExp to match if a text-field is a language field | |||
There was a problem hiding this comment.
Looks like this PR drops support for translating non-expression (token) text field values. This is a good idea to keep the library small and future-thinking, but I think we should make that change really clear in the docs. Tokens are technically still supported in GL stylesheets.
| t.end(); | ||
| }); | ||
|
|
||
| t.end(); |
There was a problem hiding this comment.
Consider adding a test case with a more complex expression (multiple get expressions, more depth).
index.js
Outdated
| var url = style.sources[sourceName].url; | ||
| // the source URL can reference the source version or the style version | ||
| // this check and the error forces users to migrate to styles using source version 8 | ||
| return url.indexOf('mapbox.mapbox-streets-v8') > -1 || /mapbox-streets-v[1-9][1-9]/.test(url); |
There was a problem hiding this comment.
geojson sources don't have a url, so should we skip sources here without a url?
index.js
Outdated
| var url = style.sources[sourceName].url; | ||
| // the source URL can reference the source version or the style version | ||
| // this check and the error forces users to migrate to styles using source version 8 | ||
| return url.indexOf('mapbox.mapbox-streets-v8') > -1 || /mapbox-streets-v[1-9][1-9]/.test(url); |
There was a problem hiding this comment.
| return url.indexOf('mapbox.mapbox-streets-v8') > -1 || /mapbox-streets-v[1-9][1-9]/.test(url); | |
| return url && url.indexOf('mapbox.mapbox-streets-v8') > -1 || /mapbox-streets-v[1-9][1-9]/.test(url); |
|
@samanpwbb I think I addressed all your concerns. This now handles the unwrapped case @andrewharvey That's great to hear that this is already working well! Once this is merged, I'm going to make a few updates and make a new release. |
README.md
Outdated
| ["match", | ||
| ["get", "name"], | ||
| "Canada", | ||
| "America's Hat", |
There was a problem hiding this comment.
hehe maybe we should use an example that is less offensive to canadians. @tristen do you have suggestions.
There was a problem hiding this comment.
["match",
["get", "name"],
"California",
"Golden State",
["coalesce",
["get", "name_en"],
["get", "name"]
]
]
This PR fixes #31 but in doing so, it drops support for all Mapbox styles based on vector tile source v7 as these styles are deprecated and Mapbox is encouraging all users to migrate to v8 styles. This change will require a major version release of this plugin.
Additionally, this PR drops support for IE11 since Mapbox GL JS is also deprecating support for the browser.
This PR also updates the example pages to use the newest styles and current versions of Mapbox GL JS and the RTL plugin while adding new example pages for Italian, Simplified Chinese and Traditional Chinese.
The code now assumes that your
text-fieldwill include an expression of the form['get', 'name_xx']. This can be a top-level expression, in acoalesceexpression or nested deeper in an expression. The plugin will also now support Italian (it), Simplified Chinese (zh-Hans) and Traditional Chinese (zh-Hant) by default. v7 style Chinesezhis no longer supported.@mapbox/gl-js