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-6303: Adding a localization preference #949

Merged
merged 30 commits into from Jan 23, 2019

Conversation

Projects
None yet
4 participants
@jobara
Copy link
Member

commented Dec 20, 2018

https://issues.fluidproject.org/browse/FLUID-6303

Adding an adjuster and enactor for localization. The enactor supports applying localization by modifying the URL path or by hooking into the lang model path. An enactment using query parameter has not been implemented yet as currently there isn't a use case for it. An example, that enacts with URL path change, has been provided.

BlueSlug and others added some commits Sep 27, 2018

@incd-ci-robot

This comment has been minimized.

@incd-ci-robot

This comment has been minimized.

@BlueSlug
Copy link
Member

left a comment

Looks pretty good from a language standpoint.
Also worth considering: updating the copyright dates where applicable

-->
<div class="fl-panelBar fl-panelBar-smallScreen">
<span class="fl-prefsEditor-buttons">
<button class="flc-slidingPanel-toggleButton fl-prefsEditor-showHide"> Show/Hide</button>

This comment has been minimized.

Copy link
@BlueSlug

BlueSlug Jan 8, 2019

Member

"Show/Hide" should be "Montrer/Cacher"

<div class="fl-panelBar fl-panelBar-smallScreen">
<span class="fl-prefsEditor-buttons">
<button class="flc-slidingPanel-toggleButton fl-prefsEditor-showHide"> Show/Hide</button>
<button class="flc-prefsEditor-reset fl-prefsEditor-reset"><span class="fl-icon-undo"></span> Reset</button>

This comment has been minimized.

Copy link
@BlueSlug

BlueSlug Jan 8, 2019

Member

"Reset" should be "Réinitialiser" or "Remettre"

This comment has been minimized.

Copy link
@jobara

jobara Jan 8, 2019

Author Member

@BlueSlug what do you think would be better to use in this case?

-->
<div class="fl-panelBar fl-panelBar-wideScreen">
<span class="fl-prefsEditor-buttons">
<button class="flc-slidingPanel-toggleButton fl-prefsEditor-showHide"> Show/Hide</button>

This comment has been minimized.

Copy link
@BlueSlug

BlueSlug Jan 8, 2019

Member

See previous comments for this and the following line

<div class="fl-panelBar fl-panelBar-wideScreen">
<span class="fl-prefsEditor-buttons">
<button class="flc-slidingPanel-toggleButton fl-prefsEditor-showHide"> Show/Hide</button>
<button class="flc-prefsEditor-reset fl-prefsEditor-reset"><span class="fl-icon-undo"></span> Reset</button>

This comment has been minimized.

Copy link
@BlueSlug

BlueSlug Jan 8, 2019

Member

ditto

@incd-ci-robot

This comment has been minimized.

@incd-ci-robot

This comment has been minimized.

enactor: {
localizationScheme: "urlPath",
langMap: {
"default": null,

This comment has been minimized.

Copy link
@cindyli

cindyli Jan 10, 2019

Member

What's the purpose of having the "default" option that does nothing when switching to it from another option?

This comment has been minimized.

Copy link
@jobara

jobara Jan 12, 2019

Author Member

Please feel free to suggest an alternative. The issue this attempts to address is when no language preference has been made. We don't want the default to be English for example, if that's the case when a user explicitly navigates to a URL like www.example.com/fr/about/ they would get redirected to the www.example.com/en/about/. By having it a no-op, it just stays where it is. Of course the downside of this is that if you had selected a localization preference and then changed to default, nothing would happen.

This comment has been minimized.

Copy link
@cindyli

cindyli Jan 14, 2019

Member

One idea I have but may not be easy to implement is, at the load of the page, is it possible to detect what the default language is used by the page, for example, by checking the lang attribute of <html> tag. Once the default language code is detected, display it along with the "default" selection item such as default (en), so from now on the "default" item can be used as a real action item for the english version of the page.

There are a couple difficulties with this idea:

  1. what if the default lang code cannot be detected by all means, such as "lang" attribute value is not set;
  2. what if the default lang code is found but it is not one of the supported localization languages.

Designers might be able to provide suggestions to this matter. I'm fine with staying with the current implementation if the brainstorming decides this is the best trade-off.

This comment has been minimized.

Copy link
@jobara

jobara Jan 15, 2019

Author Member

From what we discussed at our impromptu mini design crit, it seems that the default option is in fact as confusing as it seems. The suggestions are to change the panel description to be something more like "set your preferred language", change the "default" option to be something like "no selection", and possibly to have a discrete english option and a true no selection default.

We may also want to consider adding to this example or creating a new one that provides a language selector directly on the page in addition to the one in the panel.

}
});

fluid.defaults("example.prefsEditor", {

This comment has been minimized.

Copy link
@cindyli

cindyli Jan 10, 2019

Member

I'm not sure how useful this examplary code will help integrators. With all these uses of aux schema namespace, distributeOptions targets etc, writing this level of grade requires a good understanding of the detailed implementation of the prefs framework schema version. But of course, it's great to demonstrate how the localization can be done.

This comment has been minimized.

Copy link
@jobara

jobara Jan 12, 2019

Author Member

hmm.. that's interesting.. perhaps there is a helper grade missing that should include many of those things and could be added directly to the schema. I'll have to investigate that.

});

/**
* Used as an expander to find the langSegIndex. For most implementations this would actually refer to 1, which

This comment has been minimized.

Copy link
@cindyli

cindyli Jan 10, 2019

Member

This comment is confusing. The function is to calculate the index of the language code on the url path segments. But why would the index value 1 refers to the default value in most implementations? The default lang code in this example is null. I wonder what default lang code you expect in most implementations.

This comment has been minimized.

Copy link
@jobara

jobara Jan 13, 2019

Author Member

default is a bit of an overloaded term. The "default" preference value is null. The "default" configured langSegIndex is 1. I've tried to modify the comment to make that clearer.

inPanel: {
contextValue: "{iframeRenderer}"
},
urlPath: {

This comment has been minimized.

Copy link
@cindyli

cindyli Jan 10, 2019

Member

It seems the only localization method that the prefs framework supports is via the url path. If so, is it necessary to detect this case with the context awareness? Also, the default grade fluid.prefs.enactor.localization.no-op is not implemented. I'm curious how the localization would behave with this non-implemented grade.

This comment has been minimized.

Copy link
@jobara

jobara Jan 12, 2019

Author Member

There are effectively two implemented methods, although an integrator could support additional ones. For example using a query parameter. The two methods supported are URL path and model. The no-op is essentially the model method as there is no additional functionality needed on our part. The integrator would have to hook into the model from the enactor, which is why it uses the "fluid.resolveRoot" grade to make it more easily discoverable outside of the context of the prefs framework's component tree structure.

This comment has been minimized.

Copy link
@jobara

jobara Jan 12, 2019

Author Member

I think i just got what yo meant about the no-op grade. I believe that was something i was supposed to have removed from previous work. I've taken it out now.

* @param {String} pathname - (Optional) pathname to set. If not supplied the current pathname will be returned
* @return {String} - If the `pathname` argument is not provided, the current pathname is returned
*/
fluid.prefs.enactor.localization.urlPathLocale.pathname = function (pathname) {

This comment has been minimized.

Copy link
@cindyli

cindyli Jan 10, 2019

Member

It might be clearer to define setPathName() and getPathName() individually rather than sharing these 2 functionalities via this single function. It seems the getter is used for setting the initial value at line 70 and the setter is for responding to path change requests at line 83.

FLUID-6303: Addressing code review comments.
Largely around refactoring the example to make it easier to work with. 
This led to many refactorings in the implementation for the enactor and 
moving configuration for the constructed prefs editor into a shareable 
grade.
@incd-ci-robot

This comment has been minimized.

@jobara

This comment has been minimized.

Copy link
Member Author

commented Jan 14, 2019

@cindyli, I believe I've addressed all of your comments. Ready for more review.

@incd-ci-robot

This comment has been minimized.

FLUID-6303: rolling back testem to v2.12.0
gpii-testem currently doesn't support beyond testem 2.12.0
@incd-ci-robot

This comment has been minimized.

jobara added some commits Jan 16, 2019

FLUID-6303: Corrected some errors with LocalizationPrefsEditor config.
Including the addition of options distributions for the locale names to 
the stringArrayIndex to set the options for the panel.
FLUID-6303: Refactoring based on "design crit" feedback.
Default is now called "No Preferences". 

Also refactored urlLangSeg into a model relay.
@jobara

This comment has been minimized.

Copy link
Member Author

commented Jan 17, 2019

@cindyli this is ready for more review.

@incd-ci-robot

This comment has been minimized.

@@ -35,7 +35,10 @@ var fluid_3_0_0 = fluid_3_0_0 || {};
localeChange: {
checks: {
inPanel: {

This comment has been minimized.

Copy link
@cindyli

cindyli Jan 17, 2019

Member

What's the "inPanel" check for?

This comment has been minimized.

Copy link
@jobara

jobara Jan 18, 2019

Author Member

It's to see if we are running in the separated panel's iframe. I'm going to add a comment about that.

pathSegs.splice(langSegIndex, 1);
}
}
} else if (urlLangSeg) {

This comment has been minimized.

Copy link
@cindyli

cindyli Jan 17, 2019

Member

The "else if" check on urlLangSeg seems a repeat of the check at line 126 and unnecessary.

This comment has been minimized.

Copy link
@jobara

jobara Jan 18, 2019

Author Member

This one is for the case where hasLang is false and urlLangSeg is truthy.

"type": "fluid.prefs.localization",
"enactor": {
"type": "fluid.prefs.enactor.localization",
"classes": "@localization.classes"

This comment has been minimized.

Copy link
@cindyli

cindyli Jan 17, 2019

Member

There is not a localization.classes block defined in the aux schema. Can this line be removed?

This comment has been minimized.

Copy link
@jobara

jobara Jan 18, 2019

Author Member

thanks for catching that, I had made changes form some prior work and forgot to remove this.

@jobara

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2019

@cindyli thanks for the feedback. Ready for more review.

@incd-ci-robot

This comment has been minimized.

@jobara

This comment has been minimized.

Copy link
Member Author

commented Jan 20, 2019

@cindyli when this PR is merged, I think we should file another JIRA to take more time to examine the user experience of setting languages through the panel/adjuster. From our mini design crit there were a lot of view points expressed that made we wonder if the approach taken here is the best. For example there was mention that usually language happens through a toggle directly visible on the page and/or on a screen before hand. Which makes me wonder if a user would ever find it in the panel. We also do have a paradigm at the moment for exposing adjusters outside of the panel. Although I don't think it would be too much work to make that happen. At any rate, I think it warrants more design thought, maybe user testing and/or co-design and a more extensive exploration of how sites currently set language.

@incd-ci-robot

This comment has been minimized.

@@ -61,11 +61,13 @@
<script type="text/javascript" src="../../src/framework/preferences/js/Panels.js"></script>
<script type="text/javascript" src="../../src/framework/preferences/js/SelfVoicingPanel.js"></script>
<script type="text/javascript" src="../../src/framework/preferences/js/LetterSpacePanel.js"></script>
<script type="text/javascript" src="../../src/framework/preferences/js/LocalizationPanel.js"></script>

This comment has been minimized.

Copy link
@cindyli

cindyli Jan 21, 2019

Member

The manual test shows the localization panel added for this demo doesn't perform a language change on the page when switching to a non-English localization option.

@cindyli cindyli merged commit e62fbd9 into fluid-project:master Jan 23, 2019

2 checks passed

jenkins Build finished.
Details
license/cla Contributor License Agreement is signed.
Details
@cindyli

This comment has been minimized.

Copy link
Member

commented Jan 23, 2019

The pull request #951 is issued based on this pull request to address the last minor code review comment. #951 is merged at 35e8a5a

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.