Bug 1998347: fix user preference for language and sync with local storage #9902

nemesis09 · 2021-08-26T21:22:58Z

Epic:
https://issues.redhat.com/browse/ODC-5227

Story:
https://issues.redhat.com/browse/ODC-6326

Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=1998347

Problem/Statement:
The language preference set from the user preference page is not honored when console loads.
In the absence of a preference, the console defaults to English, ignoring "browser preference".
Waiting for user preference could also cause delay in loading of the console.

Solution/Description:
maintain the language preference in local storage in sync with the user settings.
load the console with value from local storage and update when the user settings loads
if a preference is not found in user settings or local storage, use browser preference for language as default
maintain the existing language preference from older release in the event of an upgrade

Screens:

Test Coverage:
TBD

Browser Conformance:

Firefox
Chrome
Edge
Safari

openshift-ci · 2021-08-26T21:23:05Z

@nemesis09: This pull request references Bugzilla bug 1998347, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.9.0) matches configured target release for branch (4.9.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

No GitHub users were found matching the public email listed for the QA contact in Bugzilla (gamore@redhat.com), skipping review request.

In response to this:

Bug 1998347: fix user preference for language and sync with local storage

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci · 2021-08-26T21:24:28Z

@nemesis09: This pull request references Bugzilla bug 1998347, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.9.0) matches configured target release for branch (4.9.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

No GitHub users were found matching the public email listed for the QA contact in Bugzilla (gamore@redhat.com), skipping review request.

In response to this:

Bug 1998347: fix user preference for language and sync with local storage

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci · 2021-08-26T21:24:29Z

@nemesis09: This pull request references Bugzilla bug 1998347, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.9.0) matches configured target release for branch (4.9.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

No GitHub users were found matching the public email listed for the QA contact in Bugzilla (gamore@redhat.com), skipping review request.

In response to this:

Bug 1998347: fix user preference for language and sync with local storage

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci · 2021-08-26T21:24:31Z

@nemesis09: This pull request references Bugzilla bug 1998347, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.9.0) matches configured target release for branch (4.9.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

No GitHub users were found matching the public email listed for the QA contact in Bugzilla (gamore@redhat.com), skipping review request.

In response to this:

Bug 1998347: fix user preference for language and sync with local storage

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

nemesis09 · 2021-08-26T21:27:16Z

/assign @spadgett

nemesis09 · 2021-08-27T00:26:44Z

cc @jerolimov @megan-hall

jerolimov · 2021-08-27T12:18:00Z

frontend/public/components/utils/getSystemLocale.js

+export const getSystemLocale = () => {
+  return (
+    (navigator.languages && navigator.languages.length && navigator.languages[0]) || // array of user's preferred languages ordered by preference with the most preferred language first. supported by most browsers in their latest version. https://developer.mozilla.org/en-US/docs/Web/API/Navigator/languages
+    navigator.language || // user's preferred language. fallback in case navigator.languages is not supported.
+    navigator.userLanguage // fallback compatibility for IE older versions
+  );
+};


Please make a typescript file here so that we have ?. operator and use as any hack below to support IE. Did you know which IE version we supports with this workaround?

Suggested change

export const getSystemLocale = () => {

return (

(navigator.languages && navigator.languages.length && navigator.languages[0]) || // array of user's preferred languages ordered by preference with the most preferred language first. supported by most browsers in their latest version. https://developer.mozilla.org/en-US/docs/Web/API/Navigator/languages

navigator.language || // user's preferred language. fallback in case navigator.languages is not supported.

navigator.userLanguage // fallback compatibility for IE older versions

);

};

export const getSystemLocale = () => {

return (

// array of user's preferred languages ordered by preference with the most preferred language first. supported by most browsers in their latest version. https://developer.mozilla.org/en-US/docs/Web/API/Navigator/languages

navigator.languages?.[0] ||

// user's preferred language. fallback in case navigator.languages is not supported.

navigator.language ||

// fallback compatibility for IE older versions

(navigator as any).userLanguage

);

};

jerolimov · 2021-08-27T12:21:27Z

...ages/console-app/src/components/user-preferences/language/useUserSettingsLocalStorageSync.ts

+import { useUserSettings } from '@console/shared/src/hooks/useUserSettings';
+import { deseralizeData } from '@console/shared/src/utils/user-settings';
+
+export const useUserSettingsLocalStorageSync = (


Please remove this unused file

jerolimov · 2021-08-27T12:31:59Z

frontend/packages/console-app/src/components/user-preferences/language/LanguageDropdown.tsx

+  React.useEffect(() => {
+    if (preferredLanguageLoaded && !preferredLanguage) {
+      i18n.changeLanguage(defaultLocale);
+      setPreferredLanguage(defaultPreference);
+    }
+    // run this hook only after all resources have loaded
+    // to set the preferred language user setting when the form loads
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [preferredLanguageLoaded]);
+


Please do not save the preferred language when open the user preferences this could save unexpected values only because this page is opened.

jerolimov · 2021-08-27T12:37:45Z

frontend/packages/console-app/src/components/user-preferences/language/LanguageDropdown.tsx

-    preferredLanguage ||
-    // handles languages we support, languages we don't support, and subsets of languages we support (such as en-us, zh-cn, etc.)
-    i18n.languages.find((lang) => supportedLocales[lang]);
+  const defaultPreference: string = `${PREFERRED_LANGUAGE_DEFAULT_VALUE_PREFIX}${defaultLocale}`;


Please just save null (or undefined, or nothing) until the user selects a preferred language.

jerolimov · 2021-08-27T12:40:05Z

frontend/packages/console-app/src/components/user-preferences/language/LanguageDropdown.tsx

+    const localeOptions: { [key: string]: string } = {
+      ...supportedLocales,
+      [defaultPreference]: 'Browser default',
+    };


I think its might be easier to keep selectOptions independent from the "Browser default" select option. You can maybe add a <SelectOption> just before {selectOptions} in the select? Wdyt?

Please also make "Browser default" translatable.

jerolimov · 2021-08-27T12:53:04Z

As discussed:

We should just store null as preferred language in user settings and local storage.
And we should create a new hook which fetches the preferrened language and set it to localStorage and i18n if the user use another browser which loads async the configmap but don't have a localStorage language defined

frontend/public/components/utils/getSystemLocale.js

frontend/packages/console-app/src/components/user-preferences/language/getPreferredLanguage.ts

frontend/packages/console-app/src/components/user-preferences/language/usePreferredLanguage.ts

frontend/public/i18n.js

nemesis09 · 2021-09-01T22:29:00Z

updated implementation according to latest UX designs and review comments

openshift-ci · 2021-09-01T22:35:28Z

@nemesis09: This pull request references Bugzilla bug 1998347, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.9.0) matches configured target release for branch (4.9.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1998347: fix user preference for language and sync with local storage

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

spadgett · 2021-09-02T12:24:21Z

frontend/packages/console-app/src/components/user-preferences/language/LanguageDropdown.tsx

+      <Skeleton
+        height="15px"
+        width="100%"
+        data-test={'checkbox skeleton console.preferredLanguage'}


Suggested change

data-test={'checkbox skeleton console.preferredLanguage'}

data-test="checkbox skeleton console.preferredLanguage"

spadgett · 2021-09-02T12:24:32Z

frontend/packages/console-app/src/components/user-preferences/language/LanguageDropdown.tsx

+      <Skeleton
+        height="30px"
+        width="100%"
+        data-test={'dropdown skeleton console.preferredLanguage'}


Suggested change

data-test={'dropdown skeleton console.preferredLanguage'}

data-test="dropdown skeleton console.preferredLanguage"

spadgett · 2021-09-02T12:29:30Z

frontend/public/i18n.js

-      lng: localStorage.getItem('bridge/language'),
-      fallbackLng: 'en',
+      lng: getPreferredLanguage(),
+      fallbackLng: 'ja',


Why is fallback ja?

Ahh!
I was testing and overlooked while cleaning up.

spadgett · 2021-09-02T12:29:53Z

frontend/packages/console-app/src/components/detect-language/DetectLanguage.tsx

+import { usePreferredLanguage, useLanguage } from '../user-preferences/language';
+
+const DetectLanguage: React.MemoExoticComponent<() => any> = React.memo(() => {
+  const [preferredLanguage, , preferredLanguageLoaded] = usePreferredLanguage();


Did we lose the query parameter detection for language?

I think i18next-browser-languageDetector handles that. The old language preference modal didn't seem to detect language from query params as well.
The Detect Language just populates the local storage on first load if there is a preferred language in configmap but the same does not reflect in local storage.
This will be useful if the user changes browser after setting a language preference.

spadgett · 2021-09-02T12:32:06Z

frontend/packages/console-app/src/components/user-preferences/language/usePreferredLanguage.ts

-    PREFERRED_LANGUAGE_USER_SETTING_KEY,
-    PREFERRED_LANGUAGE_LOCAL_STORAGE_KEY,
-  );
+] => useUserSettings<string>(PREFERRED_LANGUAGE_USER_SETTING_KEY, null, true);


Will this reset the language to the browser default on upgrade from 4.8 -> 4.9 since we're no longer calling useUserSettingsCompatibility?

yes it will. as discussed, updated to use useUserSettingsCompatibility and take the preference in local storage into consideration.

spadgett · 2021-09-02T14:56:49Z

frontend/packages/console-app/src/components/user-preferences/language/const.ts

@@ -4,3 +4,5 @@ export const supportedLocales = {
  ko: '한국어',
  ja: '日本語',
 };
+
+export const PREFERRED_LANGUAGE_LOCAL_STORAGE_KEY = 'bridge/preferredLanguage';


I would call this something like last-language or cached-language to make it more clear what it is. The user preference is the source of truth.

spadgett · 2021-09-02T14:58:31Z

frontend/packages/console-app/src/components/user-preferences/language/getPreferredLanguage.ts

@@ -0,0 +1,4 @@
+import { PREFERRED_LANGUAGE_LOCAL_STORAGE_KEY } from './const';
+
+export const getPreferredLanguage = (): string =>


Same here. We should rename this utility so it's clear it's not the current preference. Rather it's the last-known value.

Suggested change

export const getPreferredLanguage = (): string =>

export const getLastLanguage = (): string =>

frontend/public/components/app.jsx

megan-hall · 2021-09-02T15:12:06Z

Just a few comments. Should we keep all of the settings in a single view for now? Beau and I had discussed doing that for now since we lost most of the Language and region settings as a part of this round and wait on adding the tab and moving language until we add those in the future? If we keep the two separate tabs should the name be Language for now (since there is nothing related to region as apart of the MVP)? Can we add spacing between the check box content and the dropdown? @beaumorley do you have any other comments?

spadgett · 2021-09-02T15:22:22Z

@megan-hall I think reorganizing the tabs would need to be a follow on since this is a release blocking bug. Smaller changes like labels and spacing are probably OK to address here as long as they aren't too difficult.

beaumorley · 2021-09-02T15:39:56Z

@spadgett @megan-hall It is great that we are getting language moved to user preferences. Is there a way we could at least remove the "& region" from the tab name? Since it is just language right now?

spadgett · 2021-09-02T15:57:13Z

@spadgett @megan-hall It is great that we are getting language moved to user preferences. Is there a way we could at least remove the "& region" from the tab name? Since it is just language right now?

That shouldn't be an issue, @nemesis09 can you take a look?

nemesis09 · 2021-09-02T16:25:57Z

renamed Language & region tab to Language

spadgett · 2021-09-02T16:51:28Z

frontend/packages/console-app/src/components/quick-starts/utils/quick-start-context.tsx

@@ -174,7 +175,7 @@ export const useValuesForQuickStartContext = (): QuickStartContextValues => {
    [activeQuickStartID, setAllQuickStartStates, fireTelemetryEvent],
  );

-  const language = localStorage.getItem('bridge/language') || 'en';
+  const language = getLastLanguage() || 'en';


Not new, but en is not the right default. Can you open a separate bug to look at this?

https://bugzilla.redhat.com/show_bug.cgi?id=2001125

spadgett · 2021-09-02T16:52:04Z

frontend/packages/console-app/src/components/user-preferences/language/LanguageDropdown.scss

@@ -0,0 +1,3 @@
+.co-user-preference-language-checkbox {
+margin-bottom: var(--pf-global--spacer--sm) !important;


Not indented.

Ahh! I thought the linter handles it.
Fixed indentation.

Unfortunately, we don't use prettier or a linter for CSS yet :/ We have a story to add it.

spadgett · 2021-09-02T16:52:37Z

frontend/packages/console-app/src/components/user-preferences/language/LanguageDropdown.scss

@@ -0,0 +1,3 @@
+.co-user-preference-language-checkbox {


This should follow BEM naming, something like

Suggested change

.co-user-preference-language-checkbox {

.co-language-dropdown__system-default-checkbox {

http://getbem.com/naming/

spadgett · 2021-09-02T16:54:18Z

frontend/packages/console-app/src/components/user-preferences/language/LanguageDropdown.scss

@@ -0,0 +1,3 @@
+.co-user-preference-language-checkbox {
+margin-bottom: var(--pf-global--spacer--sm) !important;
+}


Missing final newline.

spadgett

/lgtm

openshift-ci · 2021-09-02T17:37:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nemesis09, spadgett

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [spadgett]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

nemesis09 · 2021-09-02T17:52:24Z

/retest

nemesis09 · 2021-09-02T18:49:22Z

/retest

nemesis09 · 2021-09-02T20:07:18Z

/retest

openshift-bot · 2021-09-02T20:32:32Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-ci · 2021-09-02T23:43:17Z

@nemesis09: All pull requests linked via external trackers have merged:

openshift/console#9902

Bugzilla bug 1998347 has been moved to the MODIFIED state.

In response to this:

Bug 1998347: fix user preference for language and sync with local storage

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci · 2021-09-03T00:45:10Z

@nemesis09: Bugzilla bug 1998347 is in an unrecognized state (MODIFIED) and will not be moved to the MODIFIED state.

In response to this:

Bug 1998347: fix user preference for language and sync with local storage

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci bot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Aug 26, 2021

openshift-ci bot requested review from jhadvig and rhamilto August 26, 2021 21:23

openshift-ci bot added the component/core Related to console core functionality label Aug 26, 2021

openshift-ci bot assigned spadgett Aug 26, 2021

nemesis09 force-pushed the fix-user-preference-language branch from 3aee537 to e3f1644 Compare August 26, 2021 23:51

jerolimov reviewed Aug 27, 2021

View reviewed changes

spadgett requested changes Aug 27, 2021

View reviewed changes

nemesis09 force-pushed the fix-user-preference-language branch 3 times, most recently from 74acb27 to e09a6ee Compare September 1, 2021 22:26

openshift-ci bot added the kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated label Sep 1, 2021

nemesis09 requested a review from spadgett September 1, 2021 22:28

openshift-ci bot removed the bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. label Sep 1, 2021

openshift-ci bot added the bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. label Sep 1, 2021

spadgett reviewed Sep 2, 2021

View reviewed changes

nemesis09 force-pushed the fix-user-preference-language branch from e09a6ee to fb50c4e Compare September 2, 2021 14:45

openshift-ci bot added the component/ceph Related to ceph-storage-plugin label Sep 2, 2021

spadgett reviewed Sep 2, 2021

View reviewed changes

nemesis09 force-pushed the fix-user-preference-language branch from fb50c4e to 43114c4 Compare September 2, 2021 15:38

nemesis09 force-pushed the fix-user-preference-language branch from 43114c4 to 203929d Compare September 2, 2021 16:22

spadgett reviewed Sep 2, 2021

View reviewed changes

nemesis09 force-pushed the fix-user-preference-language branch from 203929d to c9f34e4 Compare September 2, 2021 16:56

fix user preference for language and sync with local storage

ab94a64

nemesis09 force-pushed the fix-user-preference-language branch from c9f34e4 to ab94a64 Compare September 2, 2021 17:09

spadgett approved these changes Sep 2, 2021

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 2, 2021

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 2, 2021

openshift-merge-robot merged commit 93c5c99 into openshift:master Sep 2, 2021

spadgett added this to the v4.9 milestone Sep 4, 2021

	data-test={'checkbox skeleton console.preferredLanguage'}
	data-test="checkbox skeleton console.preferredLanguage"

	data-test={'dropdown skeleton console.preferredLanguage'}
	data-test="dropdown skeleton console.preferredLanguage"

		@@ -0,0 +1,4 @@
		import { PREFERRED_LANGUAGE_LOCAL_STORAGE_KEY } from './const';

		export const getPreferredLanguage = (): string =>

	export const getPreferredLanguage = (): string =>
	export const getLastLanguage = (): string =>

		@@ -0,0 +1,3 @@
		.co-user-preference-language-checkbox {
		margin-bottom: var(--pf-global--spacer--sm) !important;

	.co-user-preference-language-checkbox {
	.co-language-dropdown__system-default-checkbox {

Bug 1998347: fix user preference for language and sync with local storage #9902

Bug 1998347: fix user preference for language and sync with local storage #9902

Conversation

nemesis09 commented Aug 26, 2021 • edited

openshift-ci bot commented Aug 26, 2021

openshift-ci bot commented Aug 26, 2021

openshift-ci bot commented Aug 26, 2021

openshift-ci bot commented Aug 26, 2021

nemesis09 commented Aug 26, 2021

nemesis09 commented Aug 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerolimov commented Aug 27, 2021

nemesis09 commented Sep 1, 2021

openshift-ci bot commented Sep 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nemesis09 Sep 2, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

megan-hall commented Sep 2, 2021

spadgett commented Sep 2, 2021

beaumorley commented Sep 2, 2021

spadgett commented Sep 2, 2021

nemesis09 commented Sep 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spadgett left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Sep 2, 2021

nemesis09 commented Sep 2, 2021

nemesis09 commented Sep 2, 2021

nemesis09 commented Sep 2, 2021

openshift-bot commented Sep 2, 2021

openshift-ci bot commented Sep 2, 2021

openshift-ci bot commented Sep 3, 2021

nemesis09 commented Aug 26, 2021 •

edited

nemesis09 Sep 2, 2021 •

edited