cordova-plugin-camera icon indicating copy to clipboard operation
cordova-plugin-camera copied to clipboard

Localizing hardcoded messages

Open kohlia opened this issue 6 years ago • 18 comments

Platforms affected

iOS

What does this PR do?

Localizes messages hardcoded

What testing has been done on this change?

Works on Cordova based application

Checklist

  • [ ] Reported an issue in the JIRA database
  • [ ] Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
  • [ ] Added automated test coverage as appropriate for this change.

kohlia avatar Sep 10 '18 21:09 kohlia

Please merge this

kohlia avatar Sep 17 '18 17:09 kohlia

We use https://github.com/fairmanager-cordova/plugin-localization-strings for this. It applies pretty much universally to all native localizable strings. I like a centralized solution for this problem. Might be worth a look.

oliversalzburg avatar Sep 17 '18 20:09 oliversalzburg

We use https://github.com/fairmanager-cordova/plugin-localization-strings for this. It applies pretty much universally to all native localizable strings. I like a centralized solution for this problem. Might be worth a look.

I gather the values in InfoPlist.strings cannot be accessed in code, correct me if I am wrong

kohlia avatar Sep 17 '18 20:09 kohlia

@kohlia I'm not sure I understand the question correctly. The plugin will create localization projects from the JSON configuration. The remaining process is pretty much transparent.

If you need a localized string from ObjC, this is how to do it: https://github.com/fairmanager-cordova/plugin-barcodescanner/commit/73d0f497b242627b33dcd6fbfb32d0d14501b100

oliversalzburg avatar Sep 17 '18 20:09 oliversalzburg

@oliversalzburg makes sense so now I need to update the code to fetch from InfoPlist.strings but this will add dependency on this https://github.com/fairmanager-cordova/plugin-localization-strings plugin

kohlia avatar Sep 17 '18 20:09 kohlia

@kohlia I'm sorry, I didn't mean to suggest to depend on that plugin. I wanted to merely point out an existing solution in this area. This patch looks like it tries to achieve similar things and this approach might even conflict with existing solutions.

Ideally, all solutions play nice together. Sadly, I can't fully evaluate the situation at the moment.

oliversalzburg avatar Sep 17 '18 20:09 oliversalzburg

So what now?

kohlia avatar Sep 18 '18 15:09 kohlia

Now you can decide if this has any impact on your PR and change something or not. And then some maintainer will maybe review your PR and decide if it can and should be merged or not.

janpio avatar Sep 18 '18 15:09 janpio

Now you can decide if this has any impact on your PR and change something or not. And then some maintainer will maybe review your PR and decide if it can and should be merged or not.

This still does not impact the PR as dependency on another plugin should not be created. I feel this solution works fine

kohlia avatar Sep 18 '18 15:09 kohlia

Nobody told you to include the other plugin, @oliversalzburg just mentioned it so we all can consider if these changes might make this plugin incompatible to that other plugin that solves a similar problem for existing users.

janpio avatar Sep 18 '18 15:09 janpio

Its up to the maintainers now.

kohlia avatar Sep 18 '18 15:09 kohlia

Please merge the change

kohlia avatar Sep 27 '18 18:09 kohlia

Hey, I just fixed the problem that caused Android tests to fail in master. Could you rebase this PR please? This should get rid of the Android failures and possibly fix all test failures for this PR.

janpio avatar Oct 01 '18 13:10 janpio

Closing and re-opening to trigger a new CI/test run with new PR merge.

janpio avatar Oct 03 '18 17:10 janpio

Strings are already localizable as they are NSLocalizedString, I don't see any improvement on changing them to NSLocalizedStringFromTable

jcesarmobile avatar Nov 25 '18 12:11 jcesarmobile

That is done to specify language file, so as to keep if separate from overlapping any application language files

kohlia avatar Nov 26 '18 12:11 kohlia

I'd prefer if the literals were prefixed (as they already are in this patch) and just be left in the main table. Creating multiple tables might be the cleaner approach, but it's also a new solution in the entire environment and might require more thought in general.

oliversalzburg avatar Nov 26 '18 13:11 oliversalzburg

Hello there :wave: Any chance to see the conflict resolved? We need localization for this message but it doesn't seem possible today.

guirip avatar Apr 16 '20 13:04 guirip