-
Notifications
You must be signed in to change notification settings - Fork 2k
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
i18n: Fix translatable title strings rendering on purchases sections #49733
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~157 bytes added 📈 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
4c663d3
to
311394c
Compare
get: () => i18n.translate( 'Add Credit Card' ), | ||
}, | ||
managePurchase: { | ||
get: () => i18n.translate( 'Purchase Settings' ), |
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 appears to be untranslated when going to details from 'Active Upgrades' section.
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.
Yeah, there seems to be some inconsistency here.
If I load /me/purchases
with the flag, then go to add a card, it seems to be correct.
But then as @dlind1 mentioned, if I click on a purchase from the list, the subtitle is not translated:
Here's how it looks (translated) if I reload the purchase page:
I'm not 100% sure how flags persist, but if I load the purchase page with that flag explicitly in the URL, neither the main title nor the subtitle is translated:
Thank you for catching this. Turns out purchases controller is not subscribed to i18n changes and therefore the components are not re-rendering after loading translations asynchronously past their initial render. |
I've replaced the Could you do another round of testing and see if that resolves the problem? |
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.
@@ -40,4 +41,4 @@ ConfirmCancelDomainLoadingPlaceholder.propTypes = { | |||
selectedSite: PropTypes.oneOfType( [ PropTypes.bool, PropTypes.object ] ), | |||
}; | |||
|
|||
export default ConfirmCancelDomainLoadingPlaceholder; | |||
export default localize( ConfirmCancelDomainLoadingPlaceholder ); |
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.
I'm curious: what does localize()
do here? I thought that it just added a translate
prop, but clearly there must be some other reason it's used. Does useTranslate
have the same effect?
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.
I'm curious: what does localize() do here? I thought that it just added a translate prop, but clearly there must be some other reason it's used.
Yes, it enhances the component with the i18n props, but it also subscribes the component to the i18n's change
event and forces the component to re-render. This event is being fired when switching the locale or when adding translations to the existing Tannin locale data, as it is in our case with async loaded translation chunks.
So the problem here was that the component rendered before the translations were available in the locale data, and didn't re-render after translations were added.
Does useTranslate have the same effect?
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.
🙇♂️ Thank you for the explanation! Does this mean that it's a good idea to always use either the HOC or the hook on top-level components?
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.
🙇♂️ Thank you for the explanation! Does this mean that it's a good idea to always use either the HOC or the hook on top-level components?
I don't think it matters that much, but using the hook without assigning it to a variable may look a bit strange, imo. Essentially, it should be the same in terms of performance and behaviour.
400a9d3
to
b83e45d
Compare
Changes proposed in this Pull Request
/me/purchases
titles with getters that would fire translate function every time they're being accessed, so that titles would update properly for async loaded translations.setTitle
action dispatcher and use<DocumentHead />
instead for setting the titletitles
inlocalize
hoc, to ensure component will be re-rendered and call the translate functions after adding translation asynchronously.Testing instructions
/me/purchases?flags=use-translation-chunks
Merge instructions
Related to p1612455006017200-slack-C74C3THK7