Localizing hardcoded messages #339
Conversation
|
Please merge this |
| @@ -100,6 +100,7 @@ | |||
| <clobbers target="CameraPopoverHandle" /> | |||
| </js-module> | |||
|
|
|||
| <resource-file src="src/ios/Camera.strings" target="en.lproj/Camera.strings" /> | |||
There was a problem hiding this comment.
I don't know anything about iOS. What does target="en.lproj/Camera.strings" mean here?
There was a problem hiding this comment.
It will copy the hardcoded message file Camera.strings into a language location en.lproj as Camera.strings and we can load the message from this file in the code at runtime
There was a problem hiding this comment.
Ok, so en.lproj is a magic thing that the project will know about and load en.lproj/Camera.strings without any additional configuration?
Should the file maybe be called cordova-plugin-camera.strings?
There was a problem hiding this comment.
The en.lproj or fr.lproj or ar.lproj etc are understood by app to load corresponding language files
You are free to name the strings file just update the plugin.xml accordingly
There was a problem hiding this comment.
That was really more a question - to make sure developers know what the file is from and for. You created the PR, so you can decide and adapt or not.
There was a problem hiding this comment.
Let's leave it like this then I have no issues
src/ios/Camera.strings
Outdated
| @@ -0,0 +1,3 @@ | |||
| "camera.prohibited" = "Access to the camera has been prohibited; please enable it in the Settings app to continue."; | |||
| "camera.ok" = "Settings"; | |||
| "camera.settings" = "OK"; | |||
There was a problem hiding this comment.
These last two strings seem to be swapped
There was a problem hiding this comment.
Oh sorry about that should I update ?
There was a problem hiding this comment.
Yes, we of course can't merge obviously broken code and files.
|
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 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 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 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. |
|
So what now? |
|
Now you can decide if this has any impact on your PR and change something or not. |
This still does not impact the PR as dependency on another plugin should not be created. I feel this solution works fine |
|
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. |
|
Its up to the maintainers now. |
|
Please merge the change |
|
Hey, I just fixed the problem that caused Android tests to fail in |
|
Closing and re-opening to trigger a new CI/test run with new PR merge. |
|
Strings are already localizable as they are |
|
That is done to specify language file, so as to keep if separate from overlapping any application language files |
|
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. |
|
Hello there 👋 |
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