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

API doc for pipes #24141

Closed
wants to merge 4 commits into from
Closed

Conversation

jbogarthyde
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[x] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

Does this PR introduce a breaking change?

[ ] Yes
[x] No
 

@jbogarthyde
Copy link
Contributor Author

@petebacondarwin FYI

@mary-poppins
Copy link

You can preview f8e6cbb at https://pr24141-f8e6cbb.ngbuilds.io/.

@mary-poppins
Copy link

You can preview e3d0b77 at https://pr24141-e3d0b77.ngbuilds.io/.

@mary-poppins
Copy link

You can preview db133ab at https://pr24141-db133ab.ngbuilds.io/.

@mary-poppins
Copy link

You can preview c06c867 at https://pr24141-c06c867.ngbuilds.io/.

@mary-poppins
Copy link

You can preview dd0d78c at https://pr24141-dd0d78c.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 4b9b3c3 at https://pr24141-4b9b3c3.ngbuilds.io/.

@jenniferfell jenniferfell added area: common Issues related to APIs in the @angular/common package action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release labels Jun 14, 2018
Copy link
Contributor

@jenniferfell jenniferfell left a comment

Choose a reason for hiding this comment

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

Approved with one change: Please change to "deprecated in v5" or similar (v "from v5").

Other comments....Please file an issue against caps style in auto-generated headings. Of my other comments, the rest are minor editorial.

@@ -40,14 +48,18 @@ const unicodeWordMatch =
/(?:[A-Za-z\xAA\xB5\xBA\xC0-\xD6\xD8-\xF6\xF8-\u02C1\u02C6-\u02D1\u02E0-\u02E4\u02EC\u02EE\u0370-\u0374\u0376\u0377\u037A-\u037D\u037F\u0386\u0388-\u038A\u038C\u038E-\u03A1\u03A3-\u03F5\u03F7-\u0481\u048A-\u052F\u0531-\u0556\u0559\u0561-\u0587\u05D0-\u05EA\u05F0-\u05F2\u0620-\u064A\u066E\u066F\u0671-\u06D3\u06D5\u06E5\u06E6\u06EE\u06EF\u06FA-\u06FC\u06FF\u0710\u0712-\u072F\u074D-\u07A5\u07B1\u07CA-\u07EA\u07F4\u07F5\u07FA\u0800-\u0815\u081A\u0824\u0828\u0840-\u0858\u0860-\u086A\u08A0-\u08B4\u08B6-\u08BD\u0904-\u0939\u093D\u0950\u0958-\u0961\u0971-\u0980\u0985-\u098C\u098F\u0990\u0993-\u09A8\u09AA-\u09B0\u09B2\u09B6-\u09B9\u09BD\u09CE\u09DC\u09DD\u09DF-\u09E1\u09F0\u09F1\u09FC\u0A05-\u0A0A\u0A0F\u0A10\u0A13-\u0A28\u0A2A-\u0A30\u0A32\u0A33\u0A35\u0A36\u0A38\u0A39\u0A59-\u0A5C\u0A5E\u0A72-\u0A74\u0A85-\u0A8D\u0A8F-\u0A91\u0A93-\u0AA8\u0AAA-\u0AB0\u0AB2\u0AB3\u0AB5-\u0AB9\u0ABD\u0AD0\u0AE0\u0AE1\u0AF9\u0B05-\u0B0C\u0B0F\u0B10\u0B13-\u0B28\u0B2A-\u0B30\u0B32\u0B33\u0B35-\u0B39\u0B3D\u0B5C\u0B5D\u0B5F-\u0B61\u0B71\u0B83\u0B85-\u0B8A\u0B8E-\u0B90\u0B92-\u0B95\u0B99\u0B9A\u0B9C\u0B9E\u0B9F\u0BA3\u0BA4\u0BA8-\u0BAA\u0BAE-\u0BB9\u0BD0\u0C05-\u0C0C\u0C0E-\u0C10\u0C12-\u0C28\u0C2A-\u0C39\u0C3D\u0C58-\u0C5A\u0C60\u0C61\u0C80\u0C85-\u0C8C\u0C8E-\u0C90\u0C92-\u0CA8\u0CAA-\u0CB3\u0CB5-\u0CB9\u0CBD\u0CDE\u0CE0\u0CE1\u0CF1\u0CF2\u0D05-\u0D0C\u0D0E-\u0D10\u0D12-\u0D3A\u0D3D\u0D4E\u0D54-\u0D56\u0D5F-\u0D61\u0D7A-\u0D7F\u0D85-\u0D96\u0D9A-\u0DB1\u0DB3-\u0DBB\u0DBD\u0DC0-\u0DC6\u0E01-\u0E30\u0E32\u0E33\u0E40-\u0E46\u0E81\u0E82\u0E84\u0E87\u0E88\u0E8A\u0E8D\u0E94-\u0E97\u0E99-\u0E9F\u0EA1-\u0EA3\u0EA5\u0EA7\u0EAA\u0EAB\u0EAD-\u0EB0\u0EB2\u0EB3\u0EBD\u0EC0-\u0EC4\u0EC6\u0EDC-\u0EDF\u0F00\u0F40-\u0F47\u0F49-\u0F6C\u0F88-\u0F8C\u1000-\u102A\u103F\u1050-\u1055\u105A-\u105D\u1061\u1065\u1066\u106E-\u1070\u1075-\u1081\u108E\u10A0-\u10C5\u10C7\u10CD\u10D0-\u10FA\u10FC-\u1248\u124A-\u124D\u1250-\u1256\u1258\u125A-\u125D\u1260-\u1288\u128A-\u128D\u1290-\u12B0\u12B2-\u12B5\u12B8-\u12BE\u12C0\u12C2-\u12C5\u12C8-\u12D6\u12D8-\u1310\u1312-\u1315\u1318-\u135A\u1380-\u138F\u13A0-\u13F5\u13F8-\u13FD\u1401-\u166C\u166F-\u167F\u1681-\u169A\u16A0-\u16EA\u16F1-\u16F8\u1700-\u170C\u170E-\u1711\u1720-\u1731\u1740-\u1751\u1760-\u176C\u176E-\u1770\u1780-\u17B3\u17D7\u17DC\u1820-\u1877\u1880-\u1884\u1887-\u18A8\u18AA\u18B0-\u18F5\u1900-\u191E\u1950-\u196D\u1970-\u1974\u1980-\u19AB\u19B0-\u19C9\u1A00-\u1A16\u1A20-\u1A54\u1AA7\u1B05-\u1B33\u1B45-\u1B4B\u1B83-\u1BA0\u1BAE\u1BAF\u1BBA-\u1BE5\u1C00-\u1C23\u1C4D-\u1C4F\u1C5A-\u1C7D\u1C80-\u1C88\u1CE9-\u1CEC\u1CEE-\u1CF1\u1CF5\u1CF6\u1D00-\u1DBF\u1E00-\u1F15\u1F18-\u1F1D\u1F20-\u1F45\u1F48-\u1F4D\u1F50-\u1F57\u1F59\u1F5B\u1F5D\u1F5F-\u1F7D\u1F80-\u1FB4\u1FB6-\u1FBC\u1FBE\u1FC2-\u1FC4\u1FC6-\u1FCC\u1FD0-\u1FD3\u1FD6-\u1FDB\u1FE0-\u1FEC\u1FF2-\u1FF4\u1FF6-\u1FFC\u2071\u207F\u2090-\u209C\u2102\u2107\u210A-\u2113\u2115\u2119-\u211D\u2124\u2126\u2128\u212A-\u212D\u212F-\u2139\u213C-\u213F\u2145-\u2149\u214E\u2183\u2184\u2C00-\u2C2E\u2C30-\u2C5E\u2C60-\u2CE4\u2CEB-\u2CEE\u2CF2\u2CF3\u2D00-\u2D25\u2D27\u2D2D\u2D30-\u2D67\u2D6F\u2D80-\u2D96\u2DA0-\u2DA6\u2DA8-\u2DAE\u2DB0-\u2DB6\u2DB8-\u2DBE\u2DC0-\u2DC6\u2DC8-\u2DCE\u2DD0-\u2DD6\u2DD8-\u2DDE\u2E2F\u3005\u3006\u3031-\u3035\u303B\u303C\u3041-\u3096\u309D-\u309F\u30A1-\u30FA\u30FC-\u30FF\u3105-\u312E\u3131-\u318E\u31A0-\u31BA\u31F0-\u31FF\u3400-\u4DB5\u4E00-\u9FEA\uA000-\uA48C\uA4D0-\uA4FD\uA500-\uA60C\uA610-\uA61F\uA62A\uA62B\uA640-\uA66E\uA67F-\uA69D\uA6A0-\uA6E5\uA717-\uA71F\uA722-\uA788\uA78B-\uA7AE\uA7B0-\uA7B7\uA7F7-\uA801\uA803-\uA805\uA807-\uA80A\uA80C-\uA822\uA840-\uA873\uA882-\uA8B3\uA8F2-\uA8F7\uA8FB\uA8FD\uA90A-\uA925\uA930-\uA946\uA960-\uA97C\uA984-\uA9B2\uA9CF\uA9E0-\uA9E4\uA9E6-\uA9EF\uA9FA-\uA9FE\uAA00-\uAA28\uAA40-\uAA42\uAA44-\uAA4B\uAA60-\uAA76\uAA7A\uAA7E-\uAAAF\uAAB1\uAAB5\uAAB6\uAAB9-\uAABD\uAAC0\uAAC2\uAADB-\uAADD\uAAE0-\uAAEA\uAAF2-\uAAF4\uAB01-\uAB06\uAB09-\uAB0E\uAB11-\uAB16\uAB20-\uAB26\uAB28-\uAB2E\uAB30-\uAB5A\uAB5C-\uAB65\uAB70-\uABE2\uAC00-\uD7A3\uD7B0-\uD7C6\uD7CB-\uD7FB\uF900-\uFA6D\uFA70-\uFAD9\uFB00-\uFB06\uFB13-\uFB17\uFB1D\uFB1F-\uFB28\uFB2A-\uFB36\uFB38-\uFB3C\uFB3E\uFB40\uFB41\uFB43\uFB44\uFB46-\uFBB1\uFBD3-\uFD3D\uFD50-\uFD8F\uFD92-\uFDC7\uFDF0-\uFDFB\uFE70-\uFE74\uFE76-\uFEFC\uFF21-\uFF3A\uFF41-\uFF5A\uFF66-\uFFBE\uFFC2-\uFFC7\uFFCA-\uFFCF\uFFD2-\uFFD7\uFFDA-\uFFDC]|\uD800[\uDC00-\uDC0B\uDC0D-\uDC26\uDC28-\uDC3A\uDC3C\uDC3D\uDC3F-\uDC4D\uDC50-\uDC5D\uDC80-\uDCFA\uDE80-\uDE9C\uDEA0-\uDED0\uDF00-\uDF1F\uDF2D-\uDF40\uDF42-\uDF49\uDF50-\uDF75\uDF80-\uDF9D\uDFA0-\uDFC3\uDFC8-\uDFCF]|\uD801[\uDC00-\uDC9D\uDCB0-\uDCD3\uDCD8-\uDCFB\uDD00-\uDD27\uDD30-\uDD63\uDE00-\uDF36\uDF40-\uDF55\uDF60-\uDF67]|\uD802[\uDC00-\uDC05\uDC08\uDC0A-\uDC35\uDC37\uDC38\uDC3C\uDC3F-\uDC55\uDC60-\uDC76\uDC80-\uDC9E\uDCE0-\uDCF2\uDCF4\uDCF5\uDD00-\uDD15\uDD20-\uDD39\uDD80-\uDDB7\uDDBE\uDDBF\uDE00\uDE10-\uDE13\uDE15-\uDE17\uDE19-\uDE33\uDE60-\uDE7C\uDE80-\uDE9C\uDEC0-\uDEC7\uDEC9-\uDEE4\uDF00-\uDF35\uDF40-\uDF55\uDF60-\uDF72\uDF80-\uDF91]|\uD803[\uDC00-\uDC48\uDC80-\uDCB2\uDCC0-\uDCF2]|\uD804[\uDC03-\uDC37\uDC83-\uDCAF\uDCD0-\uDCE8\uDD03-\uDD26\uDD50-\uDD72\uDD76\uDD83-\uDDB2\uDDC1-\uDDC4\uDDDA\uDDDC\uDE00-\uDE11\uDE13-\uDE2B\uDE80-\uDE86\uDE88\uDE8A-\uDE8D\uDE8F-\uDE9D\uDE9F-\uDEA8\uDEB0-\uDEDE\uDF05-\uDF0C\uDF0F\uDF10\uDF13-\uDF28\uDF2A-\uDF30\uDF32\uDF33\uDF35-\uDF39\uDF3D\uDF50\uDF5D-\uDF61]|\uD805[\uDC00-\uDC34\uDC47-\uDC4A\uDC80-\uDCAF\uDCC4\uDCC5\uDCC7\uDD80-\uDDAE\uDDD8-\uDDDB\uDE00-\uDE2F\uDE44\uDE80-\uDEAA\uDF00-\uDF19]|\uD806[\uDCA0-\uDCDF\uDCFF\uDE00\uDE0B-\uDE32\uDE3A\uDE50\uDE5C-\uDE83\uDE86-\uDE89\uDEC0-\uDEF8]|\uD807[\uDC00-\uDC08\uDC0A-\uDC2E\uDC40\uDC72-\uDC8F\uDD00-\uDD06\uDD08\uDD09\uDD0B-\uDD30\uDD46]|\uD808[\uDC00-\uDF99]|\uD809[\uDC80-\uDD43]|[\uD80C\uD81C-\uD820\uD840-\uD868\uD86A-\uD86C\uD86F-\uD872\uD874-\uD879][\uDC00-\uDFFF]|\uD80D[\uDC00-\uDC2E]|\uD811[\uDC00-\uDE46]|\uD81A[\uDC00-\uDE38\uDE40-\uDE5E\uDED0-\uDEED\uDF00-\uDF2F\uDF40-\uDF43\uDF63-\uDF77\uDF7D-\uDF8F]|\uD81B[\uDF00-\uDF44\uDF50\uDF93-\uDF9F\uDFE0\uDFE1]|\uD821[\uDC00-\uDFEC]|\uD822[\uDC00-\uDEF2]|\uD82C[\uDC00-\uDD1E\uDD70-\uDEFB]|\uD82F[\uDC00-\uDC6A\uDC70-\uDC7C\uDC80-\uDC88\uDC90-\uDC99]|\uD835[\uDC00-\uDC54\uDC56-\uDC9C\uDC9E\uDC9F\uDCA2\uDCA5\uDCA6\uDCA9-\uDCAC\uDCAE-\uDCB9\uDCBB\uDCBD-\uDCC3\uDCC5-\uDD05\uDD07-\uDD0A\uDD0D-\uDD14\uDD16-\uDD1C\uDD1E-\uDD39\uDD3B-\uDD3E\uDD40-\uDD44\uDD46\uDD4A-\uDD50\uDD52-\uDEA5\uDEA8-\uDEC0\uDEC2-\uDEDA\uDEDC-\uDEFA\uDEFC-\uDF14\uDF16-\uDF34\uDF36-\uDF4E\uDF50-\uDF6E\uDF70-\uDF88\uDF8A-\uDFA8\uDFAA-\uDFC2\uDFC4-\uDFCB]|\uD83A[\uDC00-\uDCC4\uDD00-\uDD43]|\uD83B[\uDE00-\uDE03\uDE05-\uDE1F\uDE21\uDE22\uDE24\uDE27\uDE29-\uDE32\uDE34-\uDE37\uDE39\uDE3B\uDE42\uDE47\uDE49\uDE4B\uDE4D-\uDE4F\uDE51\uDE52\uDE54\uDE57\uDE59\uDE5B\uDE5D\uDE5F\uDE61\uDE62\uDE64\uDE67-\uDE6A\uDE6C-\uDE72\uDE74-\uDE77\uDE79-\uDE7C\uDE7E\uDE80-\uDE89\uDE8B-\uDE9B\uDEA1-\uDEA3\uDEA5-\uDEA9\uDEAB-\uDEBB]|\uD869[\uDC00-\uDED6\uDF00-\uDFFF]|\uD86D[\uDC00-\uDF34\uDF40-\uDFFF]|\uD86E[\uDC00-\uDC1D\uDC20-\uDFFF]|\uD873[\uDC00-\uDEA1\uDEB0-\uDFFF]|\uD87A[\uDC00-\uDFE0]|\uD87E[\uDC00-\uDE1D])\S*/g;

/**
* Transforms text to titlecase.
* Transforms text to title case, where the first letter of each word is capitalized.
* Capitalizes the first letter of each white-space separated word, and transforms the
Copy link
Contributor

Choose a reason for hiding this comment

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

whitespace-separated word

* Transforms text to titlecase.
* Transforms text to title case, where the first letter of each word is capitalized.
* Capitalizes the first letter of each white-space separated word, and transforms the
* rest of the word to lower-case.
Copy link
Contributor

Choose a reason for hiding this comment

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

lower case.

* in another language, you must import data for other locales.
* See the [I18n guide](guide/i18n#i18n-pipes) for instructions.
*
* WARNING: The pipe uses the Internationalization API, and is therefore
Copy link
Contributor

Choose a reason for hiding this comment

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

No comma when second clause is dependent. "The pipe uses the Internationalization API and is therefore only reliable in Chrome and Opera browsers."

*
* @usageNotes
*
* The result of this pipe is not re-evaluated when the input is mutated. To avoid the need to
Copy link
Contributor

Choose a reason for hiding this comment

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

reevaluated

* You can construct a format string using symbols to specify the components
* of a date-time value, as described in the following table.
* In JavaScript, only the components specified are respected
* (not the ordering, punctuations, and so on). Format details depend on the locale.
Copy link
Contributor

Choose a reason for hiding this comment

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

"and so on" - delete? change to "or other formatting characteristics"?

* reformat the date on every change-detection cycle, treat the date as an immutable object
* and change the reference when the pipe needs to run again.
*
* ### Pre-defined format options
Copy link
Contributor

Choose a reason for hiding this comment

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

Predefined (one word)

* These examples pipe a date into various formats,
* assuming that `dateObj` is JavaScript date object for
* year: 2015, month: 6, day: 15, hour: 21, minute: 43, second: 11.
* The output is in the local time for the 'en-US' locale.
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you want en-US in single quotes or backticks or none? Above "en-US locale" has no special treatment. Let's standardize how to handle locale names. You choose.

* of a date-time value, as described in the following table.
* In JavaScript, only the components specified are respected
* (not the ordering, punctuations, and so on). Format details depend on the locale.
* Fields marked with an asterix (*) are only available with a locale-specific data set.
*
* | Field Type | Format | Description | Example Value |
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should use sentence case in table column headers, so it matches heading cap style everywhere else in docs.

*
* ### Example
* @usageNotes
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to file an issue. Usage notes heading should be sentence caps, not title caps.

* abbreviations, but for general use, use a time zone offset (e.g. `'+0430'`).
* @param locale a `string` defining the locale to use (uses the current {@link LOCALE_ID} by
* default).
* @param value The date expression, a `Date` object or a number
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to file an issue. Input values heading should be sentence caps, not title caps.

@jenniferfell jenniferfell requested a review from vicb June 14, 2018 18:21
@jenniferfell
Copy link
Contributor

@jbogarthyde If this is no longer just to test the new template and generator, pls update the title. Thanks!

@mary-poppins
Copy link

You can preview e2b9786 at https://pr24141-e2b9786.ngbuilds.io/.

@jbogarthyde
Copy link
Contributor Author

We are still testing the template, leaving title alone.

@mary-poppins
Copy link

You can preview 1595762 at https://pr24141-1595762.ngbuilds.io/.

@mary-poppins
Copy link

You can preview de04249 at https://pr24141-de04249.ngbuilds.io/.

@jbogarthyde jbogarthyde force-pushed the jb-apidoc-pipes branch 2 times, most recently from 9a4c770 to 46097e4 Compare June 15, 2018 15:58
@mary-poppins
Copy link

You can preview 46097e4 at https://pr24141-46097e4.ngbuilds.io/.

*
*
*/
@Pipe({name: 'lowercase'})
export class LowerCasePipe implements PipeTransform {
/**
* @param value The string to transform, with words delimited by any whitespace character,
* such a space, tab, or line-feed character.
Copy link
Contributor

@vicb vicb Jun 16, 2018

Choose a reason for hiding this comment

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

I would replace

+   * @param value The string to transform, with words delimited by any whitespace character,
+   * such a space, tab, or line-feed character.

with

+   * @param value The text to convert to lower case.

I don't think trying to define what a "word" is helps and I'm not sure if this is correct definition anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not adressed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops - wrong one, this should only be in title-case. Removing it.

* Transforms text to titlecase.
* Transforms text to title case, where the first letter of each word is capitalized.
* Capitalizes the first letter of each whitespace-separated word, and transforms the
* rest of the word to lower case.
Copy link
Contributor

@vicb vicb Jun 16, 2018

Choose a reason for hiding this comment

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

Sounds like we are repeating the same thing twice ?
Replace whitespace-separated word with word as the former is wrong.

* the rest of the word into lowercase. In this case, whitespace characters (such as "space", "\t",
* "\n", etc) are used as word separators.
* @usageNotes
* The following example shows the result of transforming various strings into title case.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure how this get rendered but the text itself adds no value IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a general rule of our new API guidelines that all code examples should be introduced with a text description of what the code is an example of. In this case, it doesn't add a whole lot of value, but it is still required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most code examples show some specific thing -- it helps the reader know where to look in the code if you first tell them what it is supposed to show. This is part of reducing cognitive load for the reader, by making each point as explicit as possible.

*
*
*/
@Pipe({name: 'titlecase'})
export class TitleCasePipe implements PipeTransform {
/**
*
* @param value The string to transform, with words delimited by any whitespace character,
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as earlier. I would use @param value The text to convert to title case

Copy link
Member

Choose a reason for hiding this comment

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

For TitleCasePipe it might be valueable to explicitly mention that whitespace is used as the separator, especially since the behavior was different in the past (e.g. - where treated as boundaries, but not any more).

*/
@Pipe({name: 'uppercase'})
export class UpperCasePipe implements PipeTransform {
/**
*
* @param value The string to transform, with words delimited by any whitespace character,
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor

Choose a reason for hiding this comment

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

not adressed.

*
* The following tabled describes the formatting options.
* Only the `en-US` locale data comes with Angular. To localize dates
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

* You can construct a format string using symbols to specify the components
* of a date-time value, as described in the following table.
* Format details depend on the locale.
* Fields marked with (*) are only available if you have imported a locale-specific data set.
Copy link
Contributor

Choose a reason for hiding this comment

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

still confusing/wrong.
each locale has 2 sets of data: the "regular" one and the "extra" one. * means the extra one, this is what the former description stated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, got it.

*
* Assuming `dateObj` is (year: 2015, month: 6, day: 15, hour: 21, minute: 43, second: 11)
* in the _local_ time and locale is 'en-US':
Copy link
Contributor

Choose a reason for hiding this comment

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

in the _local_ time this was important too

vicb
vicb previously requested changes Jun 19, 2018
Copy link
Contributor

@vicb vicb left a comment

Choose a reason for hiding this comment

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

Thanks for the updates.
A few more comments

@mary-poppins
Copy link

You can preview a4f7473 at https://pr24141-a4f7473.ngbuilds.io/.

@jbogarthyde
Copy link
Contributor Author

Addressed comments, and some more small fixes.

@mary-poppins
Copy link

You can preview de33116 at https://pr24141-de33116.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 449373b at https://pr24141-449373b.ngbuilds.io/.

@jbogarthyde
Copy link
Contributor Author

@vicb @ocombe
I believe I have made all your requested changes. I think this can go green if you both remove your "changes requested" status, and vic can do pull-approve.
Travis build appears to be failing for unrelated reasons - the preview built fine.
Please let me know if there is anything else I need to do to get this landed.

@jenniferfell jenniferfell added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jun 20, 2018
@mhevery
Copy link
Contributor

mhevery commented Jun 21, 2018

@mhevery mhevery dismissed vicb’s stale review June 25, 2018 15:06

Comments addressed, Vic on vacation.

@jasonaden jasonaden closed this in d244523 Jun 25, 2018
jasonaden pushed a commit that referenced this pull request Jun 25, 2018
@cexbrayat
Copy link
Member

If I may (and @ocombe will correct me if I'm wrong), I think the docs for the currency pipe should also mention that if digitsInfo is not provided, then the format is determined by the pipe itself based on the currency: {{ 14 | currency:'CAD' }} gives $CA14.00, whereas {{ 14 | currency:'CLP' }} gives CLP14 because chilean pesos don't have cents.
Or maybe that should be in https://github.com/angular/angular/blob/master/packages/examples/common/pipes/ts/currency_pipe.ts I don't know.

I can submit a PR if you agree (unrelated, but I think there is an error in the example https://github.com/angular/angular/blob/master/packages/examples/common/pipes/ts/currency_pipe.ts#L21)

@ocombe
Copy link
Contributor

ocombe commented Jun 25, 2018

That is true, the number of digits for rounding depends on the locale

@cexbrayat
Copy link
Member

@ocombe 👍 I pushed #24661 to make it more obvious in the docs, added an example and fix an error with a current one. Let me know what you think when you have the occasion.

@jbogarthyde jbogarthyde deleted the jb-apidoc-pipes branch June 28, 2018 18:03
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: common Issues related to APIs in the @angular/common package cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants