250-change-language-fixes #275
Conversation
…250-change-language-fixes
…r not. Skip if there is no current language
| continue | ||
| } | ||
|
|
||
| val properties = "[\"coalesce\", [\"get\",\"name:$language\"],[\"get\",\"name:latin\"]]" |
There was a problem hiding this comment.
Would we maybe also want to also coalesce to just "name" as an ultimate fallback here?
There was a problem hiding this comment.
I haven't tested it, but I think otherwise non-latin-names might be completely missing from the map (if there is no latin transliteration for the name in the tiles)?
There was a problem hiding this comment.
name as final fallback sounds good in theory, but I'm not sure if something like that is working.
| fun MapboxMap.setMapLanguage(language: String) { | ||
| val layers = this.style?.layers ?: emptyList() | ||
|
|
||
| val languageRegex = Regex("(name:[a-z]+)") |
There was a problem hiding this comment.
I assume the idea here is to skip any layers with textfields that do not depend on the name from the tiles. But is also on purpose to ignore layers withtext = ["get", "name"]?
There was a problem hiding this comment.
We already ignore layers where ["get", "name"] is present, since the layer check for at least a : .
There was a problem hiding this comment.
Yeah, that's my question: is that on purpose?
There was a problem hiding this comment.
Not saying it's right or wrong, just occurred to me that I'm not sure whether a developer would expect such a layer to be localized or not...
There was a problem hiding this comment.
Thinking about it more, i think there is a good argument for either approach:
- On the one hand, if the style author sets text = ["get", "name"] they may explicitly always want to show the non-translated name here --> we should ignore these layers
- On the other hand, if the developer explicitly calls this method to localize map labels, they might expect all map labels to be localized --> these layers should be localized as well
There was a problem hiding this comment.
I would definitely go with option 1 because of multiple reasons:
- If the author just set
["get", "name"]or[ref], they want the plain name for sure. - The developer should not care about which layer will be translated and which not, since the most developer are not familiar about the different layers and how the styling is working in detail.
- The
setMapLanguagefunction would not work in our case, since we have a lot of usages of["get", "name"]and[ref], and the text is just empty in that cases.
stefanschaller
changed the title
| @@ -9,5 +9,5 @@ dependencies: | |||
| meta: ^1.0.5 | |||
|
|
|||
| environment: | |||
| sdk: '>=2.12.0 <3.0.0' | |||
| sdk: '>=2.14.0 <3.0.0' | |||
There was a problem hiding this comment.
Strictly speaking, this is a breaking change, right? Since 2.14 is almost 2 years old I think its fine, but we might want to note it in the changelog.
There was a problem hiding this comment.
Ah, maybe in general add an entry to the changelog about this PR
There was a problem hiding this comment.
Sorry about all the nitpicks, just treat them as suggestions. I'm really happy about the work you guys are doing on this repo!
There was a problem hiding this comment.
We can add a notice to the readme, besides that the PR looks very good. 👍🏽
There was a problem hiding this comment.
@m0nac0 Nitpicking like that is definitely appreciated!
|
LGTM! |
Co-authored-by: Julian Bissekkou <julian.bissekkou@tapped.dev>
No description provided.