Switch momentjs internationalisation according to I18n locale #1819
Conversation
There was a problem hiding this comment.
Impressive @leandroalemao I wouldn't expect this to be the issue.
| moment.lang('es'); | ||
| break; | ||
| case "fr": | ||
| moment.lang('fr'); |
There was a problem hiding this comment.
can't we do moment.lang(I18n.locale);? AFAIK we both (moment and OFN) use the standard acronym for languages. As it is, it would require adding a bunch of case statements more. I can think of Portugal at least cc @mllocs.
app/assets/javascripts/admin/all.js
Outdated
|
|
||
| //= require moment | ||
| //= require moment/es.js | ||
| //= require moment/fr.js |
There was a problem hiding this comment.
Doesn't require_tree do the trick? If I'm not mistaken (I'm not very familiar with the Rails asset pipeline), it'd require all the files in moment/
|
Hi @sauloperez how are you? |
|
Hi @leandroalemao many thanks for your precious time! I was just wondering if there is any way to avoid copy/pasting all that external code into OFN project and add it as a kind of dependency. What do you think? I'm not sure our |
|
Our current package.json is just to install the dependencies for the karma tests. Shouldn't the assets come with the momentjs-rails gem? They are listed in the repository: https://github.com/derekprior/momentjs-rails/tree/v2.5.1/vendor/assets/javascripts/moment I'm worried that |
|
Yep, if there's |
|
Yes, there is and we are using it already. Line 84 in 72d264f |
|
Where are we at with this, gentlemen? It's marked as critical so would be good to get it done. Does it need further changes/code review/testing? Do you need anything from the AU team to get it through? |
|
@leandroalemao @sauloperez do you have time to finish that in the coming days ? Yes it's kind of critical as frontend... Would be awesome if we could finish this off :-p |
|
Hi @ALL what do I need to include more in this PR? We're already using |
|
What I don't quite understand @leandroalemao is if we use the gem, aren't these localisation files included in it already? Do we need to upgrade its version? if so, let's do it. |
daniellemoorhead
modified the milestones:
September dot point release,
October dot point release
|
@leandroalemao do you need any help with this? |
|
Hi @sauloperez Till yesterday I was working on the PR #1836. I'll have a look at this today. Tks. Leandro. |
|
Awesome @leandroalemao no pressure! I just thought we all forgot about this (myself included). |
|
I take this over. According to https://github.com/derekprior/momentjs-rails in order to include a localization file
✅ |
|
Could you diff the size of the js bundle where this is added? 🐘 |
|
We go from 3.0MB to 3.1MB. That's already quite a lot. |
|
Watch out, I had to rebase to get the tests fixed |
|
@oeoeaio can you have a look at this one as a priority please. It's marked as critical. |
|
Yes @daniellemoorhead @oeoeaio I know I said I won't ask another issue to be reviewed in priority but when testing v1.10 I realized this was still not fixed, this is a front end untranslated thing on every French shopfront, so pretty annoying for our users... would be great if we can prioritize as well. |
What? Why?
Hi
This PR is linked to the #1107
The issue is about momentjs internationalisation.
Always glad to help.
I hope all good.
Tks.
Leandro.
What should we test?
MomentJs stuff translated according to I18n locale.
Release notes
For now, I've just added internationalisation files for Spanish and French. If the PR is ok I can add for the other languages.
Dependencies
#1107