-
Notifications
You must be signed in to change notification settings - Fork 17
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
Fix incorrect preferred locale selection with multiple languages configured #70
Conversation
// Set translations based on supported locale | ||
const locale = getSupportedLocale(); | ||
const localeData = getLocaleData( locale ); | ||
defaultI18n.setLocaleData( localeData?.locale_data?.messages ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The supported locale uses app.getLocale
that is only available when the app is read.
it( "falls back to lesser preferred languages if the most preferred isn't supported", () => { | ||
mockPreferredLanguages( [ 'mi-NZ', 'fr-FR', 'en-US' ] ); | ||
|
||
expect( getSupportedLocale() ).toBe( 'fr' ); | ||
} ); | ||
|
||
it( 'ignores region if the best match is a matching language with a different region', () => { | ||
mockPreferredLanguages( [ 'mi-NZ', 'pt-PT' ] ); | ||
|
||
expect( getSupportedLocale() ).toBe( 'pt-br' ); | ||
} ); | ||
|
||
it( "prefers an exact language-region match, even if it's lower in the preference order", () => { | ||
mockPreferredLanguages( [ 'mi-NZ', 'pt-PT', 'zh-CN' ] ); | ||
|
||
expect( getSupportedLocale() ).toBe( 'zh-cn' ); | ||
} ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We no longer need to test preferred languages as we don't use it.
it( "returns 'en' as default language", async () => { | ||
mockPreferredLanguages( [] ); | ||
mockFetchTranslations( WP_VERSION, AVAILABLE_LOCALES ); | ||
|
||
expect( await getPreferredSiteLanguage( WP_VERSION ) ).toBe( 'en' ); | ||
} ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test case is no longer needed as app.getLocale
always returns a value.
@@ -133,7 +126,7 @@ describe( 'getPreferredSiteLanguage', () => { | |||
/* NOOP */ | |||
} ); | |||
|
|||
mockPreferredLanguages( WP_5_0_LOCALES.map( ( item ) => item.locale ) ); | |||
mockAppLocale( WP_5_0_LOCALES[ 0 ].locale ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We pick the first locale but any valid locale should work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I was able to follow the testing steps and review the automated test cases successfully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, it looks cleaner now and works as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test plan succeded for me! Nice work 👍
# Conflicts: # src/lib/tests/site-language.test.ts # src/tests/site-server.test.ts
Related to https://github.com/Automattic/dotcom-forge/issues/6698.
Proposed Changes
app.getLocale
function as the requested locale to determine the supported locale. This approach supersedes the use of preferred languages, preventing the selection of an unexpected locale for the app.Testing Instructions
Preparation:
Supported languages
Unsupported languages
Pre-merge Checklist