Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

MM-12740 Move translations into redux store and remove LocalizationStore #1896

Merged
merged 6 commits into from
Oct 17, 2018

Conversation

hmhealey
Copy link
Member

@hmhealey hmhealey commented Oct 15, 2018

Saying things like "It's a fairly simple store anyway" seems to cause me a lot of problems.

This PR does a few major things:

  1. Makes it so that everything gets the locale from the redux store
  2. Moves the translation strings into the redux store
  3. Moves the loading and passing of translation strings out of the Root component and into a new IntlProvider component which wraps the one provided by react-intl

Ticket Link

https://mattermost.atlassian.net/browse/MM-12740

Checklist

  • Added or updated unit tests (required for all new features)
  • Touches critical sections of the codebase (auth, posting, etc.)

@hmhealey hmhealey added the 2: Dev Review Requires review by a core commiter label Oct 15, 2018
@hmhealey hmhealey added this to the v5.6.0 milestone Oct 15, 2018
Copy link
Member

@lieut-data lieut-data left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments, but nothing blocking. Very nice review: really easy to follow the changes :)

actions/views/root.js Show resolved Hide resolved
components/intl_provider/intl_provider.jsx Show resolved Hide resolved
$(window).unbind('storage');
}

render() {
if (this.state.translations == null || this.state.configLoaded === false) {
if (this.state.configLoaded === false) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: !this.state.configLoaded, or are we intentionally ignoring falsy but !== false values here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Not intentional.

@@ -1382,7 +1382,7 @@
"analytics.team.totalUsers": "Total Active Users",
"announcement_bar.error.email_verification_required": "Check your email at {email} to verify the address. Cannot find the email?",
"announcement_bar.error.license_expired": "Enterprise license is expired and some features may be disabled. [Please renew](!{link}).",
"announcement_bar.error.license_expiring": "Enterprise license expires on {date}. [Prease renew](!{link}).",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch!

return team && !team.delete_at > 0 && team.display_name != null;
}).
sort(sortTeamsByDisplayName);
return teams.filter((team) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could just use a expressions here:

return teams.filter((team) => (
    team && team.delete_at > 0 && team.display_name != null
)).sort((a, b) => (
    compareTeamsByDisplayName(locale, a, b)
));

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0/5 on how we do those. I'm just in the habit of always using a function body when it stretches over multiple lines

Copy link
Member

@jwilander jwilander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@hmhealey hmhealey changed the title MM-12581 Move translations into redux store and remove LocalizationStore MM-12740 Move translations into redux store and remove LocalizationStore Oct 16, 2018
@hmhealey hmhealey added Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) and removed 2: Dev Review Requires review by a core commiter labels Oct 16, 2018
@hmhealey hmhealey added 4: Reviews Complete All reviewers have approved the pull request and removed Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) labels Oct 16, 2018
@hmhealey hmhealey merged commit 02c26ca into master Oct 17, 2018
@hmhealey hmhealey deleted the mm12581b branch October 17, 2018 19:34
fincha pushed a commit to fincha/mattermost-webapp that referenced this pull request Oct 21, 2018
…ore (mattermost#1896)

* MM-12581 Move translations into redux store and remove LocalizationStore

* Switch i18n file mocking to not use identity-obj-proxy

* Address feedback
@DHaussermann DHaussermann added the Tests/Done Release tests have been written label Oct 24, 2018
@amyblais amyblais added Changelog/Done Required changelog entry has been written Docs/Not Needed Does not require documentation labels Nov 16, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Done Required changelog entry has been written Docs/Not Needed Does not require documentation Tests/Done Release tests have been written
Projects
None yet
5 participants