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

Add basic react-intl integration. #126

Merged
merged 10 commits into from Mar 22, 2020

Conversation

@patniemeyer
Copy link
Collaborator

patniemeyer commented Mar 21, 2020

No description provided.

@patniemeyer patniemeyer requested a review from SamMousa Mar 21, 2020
@SamMousa

This comment has been minimized.

Copy link
Member

SamMousa commented Mar 21, 2020

Can you fix the tests? --> you can run them locally via npm test.

@patniemeyer

This comment has been minimized.

Copy link
Collaborator Author

patniemeyer commented Mar 21, 2020

Yes, I'm looking at the failing test now.

patniemeyer added 3 commits Mar 21, 2020
@patniemeyer

This comment has been minimized.

Copy link
Collaborator Author

patniemeyer commented Mar 21, 2020

@SamMousa Tests passing now.

@SamMousa SamMousa linked an issue that may be closed by this pull request Mar 21, 2020
@patniemeyer

This comment has been minimized.

Copy link
Collaborator Author

patniemeyer commented Mar 21, 2020

Resolved the conflict.

Copy link
Collaborator

hspinks left a comment

lgtm

zh: messages_zh,
};

export const language = navigator.language.split(/[-_]/)[0];

This comment has been minimized.

Copy link
@sstur

sstur Mar 22, 2020

Collaborator

Should we use Ionic's package for extracting the language/locale? It seems to be supported across web and mobile.

This comment has been minimized.

Copy link
@SamMousa

SamMousa Mar 22, 2020

Member

Actually that's deprecated (https://github.com/apache/cordova-plugin-globalization):

With the ECMA Internationalization API now supported on iOS, Android and Windows devices, this plugin is not required any more. Migrating from this plugin to the ECMA Internationalization API is explained in this Cordova blog post.

This comment has been minimized.

Copy link
@SamMousa

SamMousa Mar 22, 2020

Member

I'm merging this now with the note that we should look into using the Internationalization spec.

@@ -1,8 +1,14 @@
import React from 'react';
import { render } from '@testing-library/react';
import App from './App';
import { IntlProvider } from 'react-intl';
import { language, messages } from './i18n/i18n';

This comment has been minimized.

Copy link
@sstur

sstur Mar 22, 2020

Collaborator

Nit, should we call this locale instead of language? Since that's how it's referred to on line 9.

Suggested change
import { language, messages } from './i18n/i18n';
import { locale, messages } from './i18n/i18n';

This comment has been minimized.

Copy link
@SamMousa

SamMousa Mar 22, 2020

Member

Fair point, I'll not block this though. Feel free to create a cleanup PR later .

This comment has been minimized.

Copy link
@patniemeyer

patniemeyer Mar 22, 2020

Author Collaborator

So the browser provides a composite field with language and optional region code suffixed. Together I think these are considered the locale. Since we are only chopping off the language prefix I called it language. But I am happy to make the change if that clarifies.

@SamMousa

This comment has been minimized.

Copy link
Member

SamMousa commented Mar 22, 2020

@patniemeyer this is good to go as soon as conflict are resolved.

@patniemeyer

This comment has been minimized.

Copy link
Collaborator Author

patniemeyer commented Mar 22, 2020

@SamMousa I've resolved the latest conflicts and re-localized the button.

@SamMousa SamMousa merged commit 60ac73f into WorldHealthOrganization:master Mar 22, 2020
5 checks passed
5 checks passed
test (ubuntu-latest)
Details
test (macos-latest)
Details
build-android-debug (ubuntu-latest)
Details
build-android-debug (macos-latest)
Details
build-ios-debug
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.