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

Avoid page reload after interface language is changed #13712

Closed

Conversation

jsnajdr
Copy link
Member

@jsnajdr jsnajdr commented May 5, 2017

The goal of this change is to avoid reloading the page after the interface language is changed and saved in the me/account form. Let's just refresh the UI in place by rerendering all React components that need to be updated!

There are several problems that need to be solved to make this work.

First, all React components that use i18n.translate to localize strings need to translate them again and rerender. That will happen if the component uses the i18n.localize HOC or the i18n.mixin mixin. Both will be notified by the i18n module and will call forceUpdate.

However, there are a few React components that call the i18n.translate function directly. (Just grep the sources to find them). These are not guaranteed to rerender. They will only if some parent calls forceUpdate, and there is no intermediate component that would return shouldComponentUpdate() == true.

Solution: migrate these i18n.translate callers to the localize HOC.

Then there are modules that call i18n.translate statically at module initializations. For these, the strings will never be translated again. I know about protect-form doing this, and there might be others.

Second, there is some language-specific info that's rendered by the server into the index page that needs to be updated dynamically on language change. This is important mainly when switching between LTR and RTL languages:

  • the <html> element has lang and dir attributes that need to be changed
  • the <body> element has an rtl class when the language is RTL
  • different stylesheets are loaded in the LTR and RTL cases. For LTR, we load style.css. For RTL, we load style-rtl.css, which is the style.css processed by RTLCSS. We'll need to reload the stylesheet at runtime, or find a way to merge them into one. (This would be much easier if more browsers supported CSS logical properties like margin-block-start or border-inline-end. But except Firefox, they don't.)

I pushed a commit that disables the page refresh on language change. It doesn't solve any of the problems mentioned above.

Removed code that triggers a refresh from the me/account form. Changing
language now behaves like changing any other setting.

Removed the redirect functionality from the me/form-base mixin, as it's
no longer used anywhere.

Also removed the 'changingUsername' property from the me/form-base state,
as it seems to be a legacy property that's not used anywhere.
@jsnajdr
Copy link
Member Author

jsnajdr commented May 8, 2017

One idea how to solve the style.css vs style-rtl.css problem: unify them into one file.

Currently, when style.css contains a rule like this:

.something {
  margin-left: 10px;
}

It's transformed by RTLCSS into a style-rtl.css rule like this:

.something {
  margin-right: 10px;
}

What about splitting the rule into two and let them all live in one file?

[dir=ltr] .something {
  margin-left: 10px;
}
[dir=rtl] .something {
  margin-right: 10px;
}

I'm not sure if RTLCSS can do this, but I assume it shouldn't be hard to write a custom PostCSS plugin from scratch that does exactly this.

@jsnajdr
Copy link
Member Author

jsnajdr commented May 16, 2017

Another place where the i18n.translate is called at initialization time and doesn't get called again on language change is the notices middleware.

@@ -525,7 +516,7 @@ const Account = React.createClass( {
onFocus={ this.recordFocusEvent( 'Interface Language Field' ) }
valueKey="langSlug"
value={ this.getUserSetting( 'language' ) || '' }
onChange={ this.updateLanguage }
onChange={ this.updateUsetSettingInput }
Copy link
Member

Choose a reason for hiding this comment

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

there is typo here updateUse*t*SettingInput

@alisterscott
Copy link
Contributor

Closing as this PR is stale - please reopen if necessary

@alisterscott alisterscott deleted the update/avoid-page-refresh-after-language-switch branch October 24, 2017 05:08
@ramonjd ramonjd restored the update/avoid-page-refresh-after-language-switch branch December 18, 2017 02:15
@alisterscott alisterscott deleted the update/avoid-page-refresh-after-language-switch branch February 1, 2018 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OSS Citizen [Size] S Small sized issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants