Skip to content
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

Closed
dmytro-gokun opened this issue Mar 9, 2019 · 10 comments
Closed

Comments

@dmytro-gokun
Copy link
Contributor

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.

@dmytro-gokun
Copy link
Contributor Author

@mourner @andrewharvey
Now, when #8012 is merged in, I'd like to work on this one. But before I go ahead and create a pull request (s), I'd like to hear your opinion on this.

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.

@andrewharvey
Copy link
Collaborator

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.

@dmytro-gokun
Copy link
Contributor Author

@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?

@andrewharvey
Copy link
Collaborator

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 uiLanguage, that means no extra weight to the build, and no extra ongoing maintenance of translations.

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.

@dmytro-gokun
Copy link
Contributor Author

@andrewharvey Okay, i see your point. I'll close the existing PR now and will work on another one.

@dmytro-gokun
Copy link
Contributor Author

@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?

@andrewharvey
Copy link
Collaborator

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.

@livthomas
Copy link

Is there any progress on this? We would very much appreciate the ability to translate Mapbox navigation controls in our application.

@dmytro-gokun
Copy link
Contributor Author

@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.

@andrewharvey
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants