Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

docs(i18n): expand the MessageFormat syntax documentation #11576

Merged
merged 1 commit into from
Apr 15, 2015

Conversation

chirayuk
Copy link
Contributor

No description provided.

@chirayuk
Copy link
Contributor Author

@petebacondarwin—I also have a bugfix for MessageFormat that needs to go in this release. I also missed the bower publish/unpublish for this module that I'll be adding (also a separate PR.)

@@ -18,8 +18,10 @@ application means providing translations and localized formats for the abstracte
Angular supports i18n/l10n for {@link ng.filter:date date}, {@link ng.filter:number number} and
{@link ng.filter:currency currency} filters.

Additionally, Angular supports localizable pluralization support through the {@link
ng.directive:ngPluralize `ngPluralize` directive}.
Localizable pluralization is supported via the {@link ng.directive:ngPluralize `ngPluralize`
Copy link
Contributor

Choose a reason for hiding this comment

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

@chirayuk do you think we should deprecate ngPluralize in 1.5 and put it in its own separate module?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hadn't done that because I had mistakenly assumed that ngPluralize allowed its embedded messages to contain HTML. Reading the docs and testing on plnkr.co shows that that is not the case.

So yes, we should definitely deprecate this for 1.5! Should I say something about it in the docs for 1.4 right now?

@petebacondarwin
Copy link
Contributor

This looks great @chirayuk!
Apart from a few minor tweaks above it looks good. Feel free to fix up and merge.


#### Selection Keywords

The selection keywords can be either exact matches or language dependant [plural
Copy link
Member

Choose a reason for hiding this comment

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

dependant --> dependent ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Fixed.

@gkalpak
Copy link
Member

gkalpak commented Apr 14, 2015

I like this (I originally thought MessageFormat would be complex to use, but it turns out it is pretty much straight-forward). 👍 @chirayuk

Apart from the minor inline comments, I have a couple (not so minor ?) not-inline comments/questions/remarks:

  1. The minified version of angular-messageFormat of the current snapshot seems to have issues (example). Using the non-minified version works fine.
    Without digging too deep, I think it is because the IIFE is invoked with window.Ga instead of window.angular. (Something related to the config of Closure compiler perhaps ?)
  2. Elsewhere, the module file is referred to as angular-messageformat[.min].js (all lowercase), but it seems to be angular-messageFormat (capital F) now. I would suggest changing it to something lowercase (either angular-messageformat or angular-message-format) to avoid issues with OSes treating file names and casing in different/weird ways.
    In any case, the name should appear consistently (and correctly) across all docs.

@chirayuk, let me know if you would like me to open separate issues.

@chirayuk chirayuk force-pushed the messageFormat_docs branch from 0eb56ef to 2ef0dde Compare April 14, 2015 21:22
@petebacondarwin
Copy link
Contributor

@gkalpak - I think your first issue has just been fixed with #11592

@petebacondarwin
Copy link
Contributor

LGTM

@petebacondarwin
Copy link
Contributor

@gkalpak for your second point, I wonder if the capitalization of the module name is being changed by dgeni when generating that page...

@chirayuk chirayuk force-pushed the messageFormat_docs branch from 2ef0dde to 383ffb7 Compare April 14, 2015 21:29
@chirayuk
Copy link
Contributor Author

@gkalpak—regarding your comments:

The minified version bug has been fixed in a PR with an E2E test for it. See PR 11592.

The casing—I fixed the only place it was inconsistently cased and made it part of this PR. However, that does not fix the problem because the stuff generated in the build/ tree contains lowercased references.

Would you file two issues for this—(1) casing should be changed to lowercase everywhere for consistency, (2) for me to create the bower repository and update scripts/bower/(un)publish.sh to use it so that this is published to bower as well. I would like to tackle those as separate issues. Thanks.

@chirayuk
Copy link
Contributor Author

On bower repo/module file name casing: I'm going to go with bower-angular-message-format and angular-message-format.min.js. based on chatting with @IgorMinar and @btford. This avoid the issue of casing that might affect case insensitive filesystem and has better consistency with names in URLs (i.e. using the hyphen as the separator character.) Let me know if you have other thoughts.

@chirayuk chirayuk force-pushed the messageFormat_docs branch 2 times, most recently from 220491c to c2c9300 Compare April 15, 2015 01:14
@gkalpak
Copy link
Member

gkalpak commented Apr 15, 2015

@chirayuk, I see you have already opened the issues.
I too think that angular-message-format is the most consistent naming.

@chirayuk chirayuk force-pushed the messageFormat_docs branch from c2c9300 to a240d72 Compare April 15, 2015 20:12
@chirayuk
Copy link
Contributor Author

@gkalpak @petebacondarwin the rename to angular-message-format is complete. It's in sha 0d64f08 (was PR #11597). I'm going to merge this one in now.

@chirayuk chirayuk force-pushed the messageFormat_docs branch from a240d72 to 1a0bcb1 Compare April 15, 2015 20:16
@chirayuk chirayuk merged commit 1a0bcb1 into angular:master Apr 15, 2015
netman92 pushed a commit to netman92/angular.js that referenced this pull request Aug 8, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants