mozilla / addons

☂ Umbrella repository for Mozilla Addons ✨
Other
127 stars 41 forks source link

Let's switch langs with the full power of Redux #10052

Closed kumar303 closed 5 years ago

kumar303 commented 7 years ago

Describe the problem and steps to reproduce it:

diff --git a/src/amo/components/LanguagePicker.js b/src/amo/components/LanguagePicker.js
index 62569e5..7d23a1f 100644
--- a/src/amo/components/LanguagePicker.js
+++ b/src/amo/components/LanguagePicker.js
@@ -3,6 +3,7 @@ import React, { PropTypes } from 'react';
 import { compose } from 'redux';
 import { connect } from 'react-redux';

+import { setLang } from 'core/actions';
 import languages from 'core/languages';
 import translate from 'core/i18n/translate';
 import { addQueryParams } from 'core/utils';
@@ -33,10 +34,7 @@ export class LanguagePickerBase extends React.Component {
     const { currentLocale, location, _window } = this.props;

     if (currentLocale !== newLocale) {
-      const newURL = changeLocaleURL({ currentLocale, location, newLocale });
-      // We change location because a locale change requires a full page
-      // reload to get the new translations, etc.
-      (_window || window).location = newURL;
+      this.props.dispatch(setLang(newLocale));
     }
   }

What happened?

Before https://github.com/mozilla/addons-frontend/pull/1627 you could see the user count change from something like '1,905 Users' to '1.905 Users'. After that PR it no longer changes.

What did you expect to happen?

I expect language changes to be visible everywhere in the app after a Redux state change.

Anything else we should know?

By using Redux we reap the following benefits (and possibly more):

None of this is a blocker for release since the lang switcher currently does a redirect.

kumar303 commented 7 years ago

For any component that needs to listen for lang changes, we can probably just provide a higher order component that provides a lang property connected to state.

mstriemer commented 7 years ago

This did change that the number is no longer modified but the strings didn't change either. The number formatting is not the problem it's that we need to modify the context to change the strings.

kumar303 commented 7 years ago

I know but the locale redirect (which reloads the page, yes) stores lang in Redux state already so why not use Redux state everywhere in the app? This will prepare us for the future when we can use Redux alone to switch locales (which seems possible to me, just with a little effort).

mstriemer commented 7 years ago

We only use the context to create the Jed instance. I have a patch that works without the moment stuff (functions aren't serializing in redux state from server, our library supports it but I left it out).

translations-without-reload mov
mstriemer commented 7 years ago

There is a partially working implementation of this in mozilla/addons-frontend#1641 but it has since bit rotted. Could be a good place to start or use for inspiration if someone wants to tackle this.

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. If you think this bug should stay open, please comment on the issue with further details. Thank you for your contributions.