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

FLUID-6299: added multilingual message bundles and defaultLocale option #917

Merged
merged 25 commits into from Sep 4, 2018

Conversation

Projects
None yet
2 participants
@BlueSlug
Copy link
Member

commented Jul 27, 2018

This work depends on outstanding unmerged work for FLUID-6305 and FLUID-6306, see pull request #913

Added multilingual message bundles for UIO for the following languages:

  • English
  • Farsi
  • French
  • Spanish

English is simply the existing bundles that have been renamed to have "_en" at the end, so there are no longer any "non-localized" message bundles.

We (@waharnum and I) have added a new top-level option on UIO to set the default locale (defaultLocale) similar to the existing options for the TOC templates and lazyload.

@BlueSlug

This comment has been minimized.

Copy link
Member Author

commented Aug 22, 2018

@jobara can you please review? thanks!

@BlueSlug

This comment has been minimized.

Copy link
Member Author

commented Aug 22, 2018

Also, some changes for FLUID-6300 have been based on the work in this branch, and further refactoring has been done. I haven't yet opened a pull request for that branch, but it's possible that some of the things you point out in review will already have been addressed.

fluid.defaults("fluid.uiOptions.prefsEditorCustomTocTester", {
gradeNames: ["fluid.test.testCaseHolder"],
modules: [{
name: "UIOptions Tests",

This comment has been minimized.

Copy link
@jobara

jobara Aug 23, 2018

Member

Should probably be "UI Options Tests".

},
{
funcName: "fluid.identity"
},

This comment has been minimized.

Copy link
@jobara

jobara Aug 23, 2018

Member

I'm assuming this call to fluid.identity is to try to get around having the two events be back-to-back. Rather than do it this way, can you just do all the tests after the last event. If you really need to listen and run tests to the different events independently and you are not able to just specify both events, please make sure to leave a comment and link to the relevant JIRA issue for why this is necessary.

This comment has been minimized.

Copy link
@jobara

jobara Aug 23, 2018

Member

Also, it appears that this test could just use the plain jqUnit.async test instead. You'd have separate listeners to each event and run jqUnit.start() in the listener that executes last.

fluid.defaults("fluid.uiOptions.prefsEditorLocalizedTester", {
gradeNames: ["fluid.test.testCaseHolder"],
modules: [{
name: "UIOptions Locale Tests",

This comment has been minimized.

Copy link
@jobara

jobara Aug 23, 2018

Member

Similar to above, rename to "UI Options"

var customizedTocTemplate = "../../../../src/components/tableOfContents/html/TableOfContents.html";

/* Mixin grade for UIO test component */
fluid.defaults("fluid.uiOptions.testPrefsEditorBase", {

This comment has been minimized.

Copy link
@jobara

jobara Aug 23, 2018

Member

Better to put all of the test grades, functions, and etc in the "fluid.tests" namespace. This should ensure that there are no accidental collisions with the actual component and functions under test, and keeps things semantically grouped together in the global object.

}]
});

var localizedValuesToVerify = {

This comment has been minimized.

Copy link
@jobara

jobara Aug 23, 2018

Member

This should be added to the fluid object under the appropriate namespace. We try to keep all the parts publicly available.

fluid.uiOptions.prefsEditorLocalizedTester.verifyLocalizedMessages = function (prefsEditor) {
fluid.each(prefsEditor.prefsEditorLoader.messageLoader.resources, function (panel, key) {
var actualValue = panel.resourceText[localizedValuesToVerify[key].path];
jqUnit.assertEquals("Panel " + key + " localized message loaded correctly", localizedValuesToVerify[key].expected, actualValue);

This comment has been minimized.

Copy link
@jobara

jobara Aug 23, 2018

Member

This doesn't seem to test that the localized values are actually rendered.

@jobara

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

Could you also localize the captions.json message bundle?

@jobara

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

The prefsFramework demo and some of the examples have their own message bundles. Should these be specified with the "en" locale?

@jobara

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

I'm thinking a bit more about there no longer being any "non-localized" fall backs. If an integrator supplies a defaultLocale that doesn't exist, the component will break. Is this what we want? Let me know what you think. Also should we specify the specific locales for the language, will it still get picked up if the integrator requests a different version of that locale? Also maybe we should go ahead and have separate localizations for en-us and en-ca, in particular for things like "colours vs colors".

@BlueSlug

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2018

Thanks for your review so far, @jobara. I'm making changes to address the items you've mentioned. So far I've:

  • updated the test names to "UI Options" instead of "UIOptions"
  • added non-localized message files back into the project
  • added en_CA and en_US files to the project, where en_CA is simply a duplicate of the en and the non-localized bundles (the only differences I noticed were in the spelling of "grey/gray" and "colour/color")
  • added new bundles for the captions panel in French, Farsi, and machine translated Spanish
  • added all variables in the test file to the "fluid.tests.uiOptions" namespace

I still have to:

  • verify that the rendered strings match the stored strings after a locale change
  • check out how to use jqUnit.asyncTest in this context. I need to do a bit of reading to fully understand that and why using fluid.identity is not ideal where I've put it

Other comments:

  • before adding the non-localized bundle back in, I confirmed that the panel broke when the locale was set to a value that wasn't present in the existing bundles. For the sake of avoiding regressions or issues with legacy projects, I've taken your advice.
  • an integrator could specify a locale with region included (like "fr_CA"), and if that particular locale doesn't exist the fallback rules will load the next best thing ("fr" in this case)
  • perhaps an en_GB bundle could be added as well (differences in words like "Emphasize/Emphasise")
@BlueSlug

This comment has been minimized.

Copy link
Member Author

commented Aug 30, 2018

@jobara Thanks for your help in moving this along. I've made the remaining changes you requested

@@ -5,7 +5,7 @@
"contrast-by": "Black on yellow",
"contrast-yb": "Yellow on black",
"contrast-lgdg": "Light grey on dark grey",
"contrast-gw": "Grey on white: ",
"contrast-gw": "Grey on white",

This comment has been minimized.

Copy link
@jobara

jobara Sep 4, 2018

Member

thanks for catching this typo

"description": "",
"increaseLabel": "",
"decreaseLabel": ""
}

This comment has been minimized.

Copy link
@jobara

jobara Sep 4, 2018

Member

It seems that the translations for this adjuster are missing.

This comment has been minimized.

Copy link
@BlueSlug

BlueSlug Sep 4, 2018

Author Member

I spoke with Sepi about this one, and while I initially thought it was an oversight, I'd done it intentionally. It turns out that the idea of letter spacing in Farsi doesn't really apply, and I didn't want it to fall back to another localization, so I added the blank strings with the anticipation that we would be working on the ability to activate/deactivate certain preferences by localization at some point (I believe there is a Jira for this but I can't find it at the moment). I wanted to add a comment in the file but Atom didn't like it, so I didn't.

What do you think is the best solution for this issue?

This comment has been minimized.

Copy link
@jobara

jobara Sep 4, 2018

Member

It's a good question. I think it's probably better to have the localized text, otherwise the UI will just look broken. And the adjustments would still apply if they tried to use it. Probably better that they know what it does. Also, if originally they set the letter spacing for a different language at least they'd know where to go to change it back. That's just my thoughts.. I think long term we'd also need to know what languages not to apply settings too, as the letter spacing could already be part of their prefs set.

"label": "Enhance Inputs",
"description": "Emphasize links, buttons, menus, textfields, and other inputs",
"switchOn": "on",
"switchOff": "off"

This comment has been minimized.

Copy link
@jobara

jobara Sep 4, 2018

Member

There seems to be some inconsistency for adjusters that have a the on/off switch. In some message bundles the values ore ON/OFF and others on/off. Could you please update them to be consistent. The rendered casing is probably handled by CSS, but it's worthwhile to double check.

@BlueSlug

This comment has been minimized.

Copy link
Member Author

commented Sep 4, 2018

@jobara ready for further review! :)

@jobara jobara merged commit 339c4b5 into fluid-project:master Sep 4, 2018

1 check passed

license/cla Contributor License Agreement is signed.
Details
@jobara

This comment has been minimized.

Copy link
Member

commented Sep 4, 2018

merged at 780f34d

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.