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.

Sign up for GitHub

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

Jump to bottom

import locale for angular #6843

Merged
merged 1 commit into from
Dec 11, 2017
Merged

import locale for angular #6843

merged 1 commit into from
Dec 11, 2017

Conversation

ctamisier
Copy link
Contributor

@ctamisier ctamisier commented Dec 10, 2017 •
edited
Loading

Fix for #6836. Let's see about the coding rules (using a constructor ?)

check 'Pipes, i18n and breaking changes' in http://blog.ninja-squad.com/2017/11/02/what-is-new-angular-5/

  • Travis tests are green (waiting for travis)
  • Tests are added where necessary (no need I think...)
  • Documentation is added/updated where necessary (no need I think...)
  • Coding Rules & Commit Guidelines as per our CONTRIBUTING.md document are followed

@pascalgrimaud
Copy link
Member

Thanks @ctamisier
I will test it, specially for languages like ar_ly, zh_cn, etc

@ctamisier
Copy link
Contributor Author

ctamisier commented Dec 10, 2017 •
edited
Loading

thanks for testing, good point because it's not working with:
zh-tw, zh-cn, pt-br and ua.
Which seems normal because language doesn't mean locale.

What I think (from the table in https://angular.io/guide/i18n#setting-up-the-locale-of-your-app):
zh-cn => import locale from '@angular/common/locales/zh-Hans';
zh-tw => import locale from '@angular/common/locales/zh-Hant';
pt-br => import locale from '@angular/common/locales/pt'; (angular/angular#20197 (comment))
ua => import locale from '@angular/common/locales/uk'; (maybe we should use uk instead of ua as language key)

@ctamisier
Copy link
Contributor Author

Or maybe we should just update the values of LANGUAGES in https://github.com/jhipster/generator-jhipster/blob/master/generators/generator-constants.js to make them match with the angular localeID.

deepu105
deepu105 requested changes Dec 10, 2017
View reviewed changes
generators/client/templates/angular/src/main/webapp/app/shared/_shared-common.module.ts Outdated
@@ -18,6 +18,16 @@
-%>
import { NgModule, LOCALE_ID } from '@angular/core';
import { Title } from '@angular/platform-browser';
import { registerLocaleData } from '@angular/common';
<%_ const localeIds = {
'zh-cn': 'zh-Hans',
Copy link
Member

Choose a reason for hiding this comment

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

I suggest to add a new attribute to the original language object in generator-constants so that this is not hidden, then fetch the value from that object lile how we do for rtl

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

generators/client/templates/angular/src/main/webapp/app/shared/_shared-common.module.ts
export class <%=angularXAppName%>SharedCommonModule {}
export class <%=angularXAppName%>SharedCommonModule {
constructor() {
registerLocaleData(locale);
Copy link
Member

Choose a reason for hiding this comment

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

Just ensure that this works fine with AOT as it has history of not working with normal js constructs sometimes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hum :), I don't know what this really means... can you guide me a bit on this ? thx

Copy link
Member

Choose a reason for hiding this comment

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

just run the app using ./mvnw -Pprod and make sure everything works fine, as this will use AOT and if there is an issue here it should fail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So everything seems to work fine with this. Running ./mvnw -Pprodcompiles and runs with no problem. Then in the app everything is fine as well, I tried with different locales, so it's all good for me.

@ctamisier
Copy link
Contributor Author

yeah it's indeed a better idea.
I added localeId in LANGUAGES and created a function getLocaleId(language).

There is the case of ar-ly which is skipForLocale and then fallback to 'en' as a localeId (but there is a file @angular/common/locales/ar-ly), so maybe we should remove this behavior ?

@deepu105
Copy link
Member

@ctamisier for ar-ly the default locale causes some issue in dates thats why the override, i'm not sure if the issue is fixed now, can you test ar-ly with some all date types in entities?

@ctamisier
Copy link
Contributor Author

ctamisier commented Dec 11, 2017 •
edited
Loading

If we use ar-ly as localeId (not what we currently have):
<td>{{user.createdDate | date}}</td> shows 2017/12/11 which is OK.

Then if we switch to english language it shows 112017/12/ (actually 11&rlm;/12&rlm;/2017) but because we switch back to LTR, it doesn't make sense (it's what I think...).

It wouldn't be a problem if we could change the localeId runtime which not doable today (angular/angular#15039).

So I left thear-ly and skipToLocale with no change.
I don't have further modification to bring to this PR

@deepu105 deepu105 merged commit a1553db into jhipster:master Dec 11, 2017
@ctamisier ctamisier deleted the locale branch December 11, 2017 23:14
@pascalgrimaud pascalgrimaud mentioned this pull request Dec 14, 2017
Closed
1 task
@jdubois jdubois added this to the 4.13.0 milestone Dec 14, 2017
@marcelinobadin
Copy link
Contributor

Just to aggregate some info here, I'm from Brazil and therefore I am always using pt-BR and the solution I have found was to change from pt-br to pt-BR everywhere because there is a case sensitive matter on the language code. Also using only 'pt' is more common use to only portuguese from Portugal which is quite different from brazilian portuguese and we will keep using pt-BR here to not use funny words that we don't use as Portugal people does heheheheeh.

@ctamisier
Copy link
Contributor Author

thanks @marcelinobadin for your input.
Actually in JHipster there are 2 things related to languages:

  1. The JHipster language which is a code for JHipster to know what language the user is using. It is internal and not a standard. For Brazilian it's pt-br
  2. The LOCALE_ID which is required for Angular and specially to load the locale file. For Brazilian it's this file https://github.com/angular/angular/blob/master/packages/common/locales/pt.ts (you can see indide the currency is Real brasileiro), the LOCALE_ID is pt.
    There is no pt-BR in https://github.com/angular/angular/tree/master/packages/common/locales,
    and then for Portugal the LOCALE_ID is pt-PT.

I guess Angular decided to design this way.. or I might misunderstand something ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deepu105 deepu105 deepu105 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
4.13.0
Development

Successfully merging this pull request may close these issues.
5 participants
@ctamisier @pascalgrimaud @deepu105 @marcelinobadin @jdubois