-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Abilitry to set tooltips on standard controls/change them on the fly. #8010
Comments
@mourner @andrewharvey First of all, is it better to have a separate pull request per a control or do them all at once? (I'd like to start with FullscreenControl first, just to use that as a prototype for the rest of controls). Secondly, my idea was to add new optional parameters to controls' constructors. Do you like it? Another idea was to give ability to change the tooltip(s) after the control is created (this is for advanced scenarios, like when UI language is changed during the app's lifetime). But after some thinking, i decided to leave this out for now. So, plz let me know what you think here. If you are okay with the idea, I'll go ahead and create a PR. |
I'm 👎 to implementing fine grain ability to set tooltip values. These controls are standard out of the box features and the default tooltips should be sufficient. If you're trying to re-purpose the controls to do something else, best to create a new control entirely. If it's for language localisation, I think it's better we implement that across the library, rather than by passing strings directly like this. For example this could be a uiLanguage option to Map, and we have a fixed table of language strings to lookup, or the user could provide their own translation by passing an object to uiLanguage. |
@andrewharvey You are right, the main purpose here is localization. We want to support different languages in our app and mapbox kind of sticks out with its English-only text. The approach you're suggesting is pretty neat and clean. However, it looks like it is going to be a pretty big task for a single PR. May be it has to be split into several steps. A concern here is the library size. If we're going to support a bunch of languages and have a bunch of strings to translate, all that may end up increasing bundle size significantly. Even for those customers who are not interested in particular languages. Then, may be it's better to have a separate package per UI language? Another issue I can see with this approach is that it is pretty hard to support. Say, we have support for 50 languages and then we need to add one new string to the UI. Then what about custom controls. Should they use this central "string DB" (may be, by extending it on the fly?) or use their own localization strategy? |
I count about 8 strings which could be localised, so it's not a huge amount, I think one self contained PR is best, but I think we ultimately need to see what the maintainers would accept. Could start with just English, but with the structure in place, anyone can pass in their own language translations through mapbox-gl-geocoder a custom control just did this in https://github.com/mapbox/mapbox-gl-geocoder/pull/201/files Though the control interface could pass a localisation setting across to the control. |
@andrewharvey Okay, i see your point. I'll close the existing PR now and will work on another one. |
@andrewharvey Does this PR follow the idea you were suggesting? Or did I misunderstand it? Let's have some discussion here and decide on the route to take. I think having ability to localize UI easily is a very good thing to have for the library. Is not it? |
Yeah I think it's worthwhile to add this feature, and I think your PR is great work! I'll leave a couple comments on the PR, but I'll still defer a proper review to someone on the core team, they should get to it, just may take some time. |
Is there any progress on this? We would very much appreciate the ability to translate Mapbox navigation controls in our application. |
@livthomas I do not think this PR is going to be accepted. There is another one here: #8095 I've just saw that a reviewer left some comments. Somehow, github did not notify me about that fact (or i've missed the notification). I'll try to resolve those comments during near days. |
I think it's fair to say this has been addressed now by #8095 so I'll close this ticket, a new issue can be opened for other feature requests or issues. |
This will give us ability to translate our apps properly w/o need to hack into the standard controls's guts and changing attributes on them.
The text was updated successfully, but these errors were encountered: