Localise user count in search results #1627
Conversation
| @@ -71,7 +71,7 @@ export function unexpectedSuccess() { | |||
| /* | |||
| * Creates a stand-in for a jed instance, | |||
| */ | |||
| export function getFakeI18nInst() { | |||
| export function getFakeI18nInst({ lang = 'en-US' } = {}) { | |||
There was a problem hiding this comment.
I know this is only a test, but do we declare en-US as the default locale anywhere else? Would be nice to use a constant rather than the magic one here.
| const localisedRoot = renderResult(result, { lang: 'fr' }); | ||
| const users = findRenderedDOMComponentWithClass(localisedRoot, 'SearchResult-users'); | ||
| // \xa0 is a non-breaking space. | ||
| assert.match(users.textContent, /5\xa0253/); |
There was a problem hiding this comment.
L'espace est bizarre ici pour moi.
How many results?
"5 253 !"
English must feel so cramped in comparison. 😉
|
|
||
| // This adds the correct moment locale for the active locale so we can get | ||
| // localised dates, times, etc. | ||
| if (i18n.options && typeof i18n.options._momentDefineLocale === 'function') { | ||
| i18n.options._momentDefineLocale(); | ||
| moment.locale(makeMomentLocale(i18n.options.locale_data.messages[''].lang)); | ||
| moment.locale(makeMomentLocale(i18n.lang)); |
There was a problem hiding this comment.
That is quite a bit nicer, cheerio 👍
|
Actually, only thing is: do we want to keep all our Just a thought. |
|
I like the idea of |
|
I'd like to preserve the ability to dispatch a Redux action and change I know that our current language switcher doesn't work this way since it does a redirect but that's a hack IMO and we should fix it. Redux is made for state changes like this. |
|
Maybe if |
| @@ -187,6 +187,8 @@ export function makeI18n(i18nData, lang, _Jed = Jed) { | |||
| const i18n = new _Jed(i18nData); | |||
| i18n.lang = lang; | |||
|
|
|||
| i18n.formatNumber = (number) => number.toLocaleString(lang); | |||
|
We'd need to load the new i18n data and likely create a new The react docs [1] suggest that you do not update things that are in context because a parent component can return They link to an article with potential solutions but I haven't read through it. Sounds like there is going to be some tricky bits getting the app to re-render after a lang change since we have that in context. Either way I don't think these changes prevent, or make more difficult, updating the app when the lang changes. [1] https://facebook.github.io/react/docs/context.html#updating-context |
|
Yes, the blog post you linked to says not to do exactly what you did :) None of this is a blocker for release but I think this patch would have been just as easy to write using Redux state. I'd like to fix it in the future because Redux will make the app a lot easier to work on. Filed: mozilla/addons#10052 (low priority) |
|
To be clear: I know there are other challenges preventing us from ditching the locale redirect (like, how do we load the new translations?) but the redirect was already hooked up to Redux state so we might as well use it to prepare for the future. |
|
I didn't modify the context. I changed a value that it had when it was created, that is totally fine. If we want to make lang switching without a full reload possible then we will need to modify the |
I updated how the lang is acquired. Rather than using state it uses
i18n.langand the lang is passed to themakeI18n()function to be set rather than pulling it from the first message (I'm curious what that would do if the string wasn't translated or we maybe didn't have any translations for a locale).I updated the string to match the one from
AddonMetasince they have different names for the number so it would have caused there to be an extra translation string (I think).Screenshots:
English
French
Fixes mozilla/addons#10040.