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

[MNOE-206] Add pricing details to dock settings #82

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

hedudelgado
Copy link
Contributor

tooltip-placement="bottom" tooltip="This is the total number of people in your organization using {{app.name}}"></ng-pluralize>
</div>
<div ng-show="!app.freeTrialEndAt && !app.perUserLicence">Pricing details inside your app</div>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use i18n keys.

<div ng-show="!!app.freeTrialEndAt">
Free Trial: {{ app.freeTrialEndAt }}
</div>
<div ng-show="!app.freeTrialEndAt && app.perUserLicence">
Copy link
Contributor

Choose a reason for hiding this comment

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

Where are app.freeTrialEndAt and app.perUserLicence defined?
Are they supposed to come from the API? (In this case the variables would be snake cased)

@hedudelgado
Copy link
Contributor Author

Please @alexnoox , have a look at the changes.

Thank you very much for your comments.

Copy link
Contributor

@alexnoox alexnoox left a comment

Choose a reason for hiding this comment

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

LGTM.

@ouranos FYI

@@ -28,6 +28,7 @@ angular.module 'mnoEnterpriseAngular'
# ----------------------------------------------------------
$scope.helper = {}
$scope.helper.canDeleteApp = ->
console.log(app)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove debug logs ;)

@hedudelgado
Copy link
Contributor Author

I cleaned it up @ouranos @alexnoox.

@hedudelgado hedudelgado changed the title Add pricing details to dock settings [MNOE-206] Add pricing details to dock settings Oct 19, 2016
</div>
<div ng-show="!app.free_trial_end_at && app.per_user_licence">
<ng-pluralize count="app.licences_count" when="{'0': 'No users', '1': '1 user', 'other': '{{app.licences_count | number}} users'}"
tooltip-placement="bottom" tooltip="This is the total number of people in your organization using {{app.name}}"></ng-pluralize>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please i18n this too.
Ask @clemthenem, he worked on pluralisation and i18n ;)

@hedudelgado hedudelgado force-pushed the pricing branch 3 times, most recently from 8abdc07 to 4b509d8 Compare October 27, 2016 15:40
{{ 'mno_enterprise.templates.impac.dock.settings.free_trial' | translate }}: {{ app.free_trial_end_at }}
</div>
<div ng-show="!app.free_trial_end_at && app.per_user_licence">
<ng-pluralize count="app.licences_count" when="{'0': '{{ &quot;mno_enterprise.templates.impac.dock.settings.no_users&quot; | translate }}', '1': '{{ &quot;mno_enterprise.templates.impac.dock.settings.1_user&quot; | translate }}', 'other': '{{app.licences_count | number}} {{ &quot;mno_enterprise.templates.impac.dock.settings.users&quot; | translate }}'}"
Copy link
Contributor Author

@hedudelgado hedudelgado Oct 27, 2016

Choose a reason for hiding this comment

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

Heys @alexnoox , this is the best solution I found to solve the problem of the i18n within the ng-pluralize. I have not checked with @clemthenem as I thought this could be enough. Please confirm this is ok, otherwise I will look for another different or ask clement straight away. In case it works for you, it should be ready to merge. I also added the empty i18n Keys with empty values for different languages for the dock, they were not in the project before.

Thanks!

Copy link
Contributor

@clemoberti clemoberti Nov 9, 2016

Choose a reason for hiding this comment

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

I would suggest you add the MessageFormat plugin, using the bower command:
bower install angular-translate-interpolation-messageformat

Then add it to your mno-enterprise-angular/src/app/index.config.coffee file:
$translateProvider.addInterpolation('$translateMessageFormatInterpolation');
This means that it will use default ng-translate functionalities and only load this plugin when required.

You can then add this translation in the en.json file:
mno_enterprise.templates.impac.dock.settings.nb_of_users: '{num_users, plural, =0{No users} =1{1 user} other{# users}}';

  1. 'num_users' - The name of the variable you are using to test the number
  2. 'plural' - The type of action you want to use ( it could also be 'gender' for example)
  3. All the different cases (# is replaced by the variable value in your translation).

Then you can simply use it like this in your template
<p translate="mno_enterprise.templates.impac.dock.settings.nb_of_users" translate-values="{ num_users: app.licences_count }" translate-interpolation="messageformat"></p>

If you wanna play around with it I made this plunkr:
http://plnkr.co/edit/GyrLvacl7rk6GgsH0S6o?p=preview

Let me know if you have any problem.
Here is the doc: https://angular-translate.github.io/docs/#/guide/14_pluralization

@ouranos ouranos assigned alexnoox and hedudelgado and unassigned alexnoox Nov 10, 2016
@hedudelgado
Copy link
Contributor Author

@alexnoox I have added the solution that @clemthenem suggested. I must thank you by the way Clement, it was pretty clear and straight forward.
Thanks a lot.

MichaelSedov pushed a commit to MichaelSedov/mno-enterprise-angular that referenced this pull request Jan 26, 2017
Copy link
Contributor

@ouranos ouranos left a comment

Choose a reason for hiding this comment

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

The mnoe API still uses time_ago_in_words. I though we decided to use ISO8601 format and humanise it in the frontend?

bower.json Outdated
@@ -23,6 +23,11 @@
"impac-angular": "~1.5.0-rc1",
"jquery": "~3.1.0",
"less": "~2.5.3",
"angular-translate": "~2.9.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

These lines are duplicated

Copy link
Contributor

@alexnoox alexnoox left a comment

Choose a reason for hiding this comment

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

In the id.locale.json and zh.locale.json, you don't need to insert empty locales keys, the framework will automatically fallback to english if the key is not found.

@hedudelgado
Copy link
Contributor Author

@alexnoox updated.

@hedudelgado hedudelgado force-pushed the pricing branch 4 times, most recently from e3c6931 to 98527d4 Compare March 10, 2017 18:22
Copy link
Contributor

@ouranos ouranos left a comment

Choose a reason for hiding this comment

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

@alexnoox could do a final review?

The bower.json is malformed

bower.json Outdated
"moment": {
"main": [
"moment.js",
"locale/id.js"
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing a ,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@alexnoox alexnoox left a comment

Choose a reason for hiding this comment

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

@hedudelgado Could you rebase this PR on master?

Also use angular-moment instead of moment directly.

@@ -46,6 +47,17 @@
"less/font-awesome.less",
"./fonts/*"
]
},
"moment": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you need to do this? We already import angular-moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @alexnoox
In angular moment docs:

Usage
Include both moment.js and angular-moment.js in your application.

My input
This solution is out of the box and works by loading the different locales files we need in the project from the bower.json. The best part is that we can load all of them, or also only the ones we need. I specified id and ch to avoid having unnecessary extra files, but it can be extended anytime!
"moment": {
"main": [
"moment.js",
"locale/*.js"

Once it is loaded, by passing the locale we are using to moment in the run, or in the select language box for changing it.

moment.locale(TheLocaleWeAreUsing)

The language will be set globally and therefore propagated to every bit in the code where we are binding the result of moment.
The default language is English, it does not need to be loaded.
I tried with Chinese, french and Indonesian and seems to work perfectly fine, so it is very easy to extend.

@x4d3
Copy link
Contributor

x4d3 commented Sep 6, 2017

@alexnoox @hedudelgado Would it be possible to merge this ?

@alexnoox
Copy link
Contributor

alexnoox commented Sep 7, 2017

@x4d3 No we can't merge this now.

This PR derived from its original goal ([MNOE-206] Add pricing details to dock settings) to i18n and l10n of dates on mno-enterprise-angular. Those features need to be addressed in 2 PRs, especially after the recent work of @ouranos on i18n.

The i18n and l10n solution with angular-moment looks good, it needs to be reworked a bit.

  • You are using directly using moment (moment.locale($scope.selectedLangKey)) where you sould use the amMoment service (cf. https://github.com/urish/angular-moment#usage).
  • In the bower file, I don't think we need to load moment.js as it is already loaded. We will need to load the moment locales though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants