API doc for pipes by jbogarthyde · Pull Request #24141 · angular/angular

jbogarthyde · 2018-05-25T21:46:15Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

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 · 2018-05-25T21:51:05Z

@petebacondarwin FYI

mary-poppins · 2018-05-25T22:00:02Z

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

mary-poppins · 2018-05-25T22:04:24Z

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

mary-poppins · 2018-06-13T19:05:34Z

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

mary-poppins · 2018-06-13T19:34:56Z

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

mary-poppins · 2018-06-13T23:33:34Z

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

mary-poppins · 2018-06-14T15:28:47Z

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

jenniferfell

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.

jenniferfell · 2018-06-14T17:28:17Z

whitespace-separated word

jenniferfell · 2018-06-14T17:28:28Z

lower case.

jenniferfell · 2018-06-14T17:29:51Z

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

jenniferfell · 2018-06-14T17:30:35Z

reevaluated

jenniferfell · 2018-06-14T17:32:59Z

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

jenniferfell · 2018-06-14T17:51:36Z

Predefined (one word)

jenniferfell · 2018-06-14T17:52:47Z

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.

jenniferfell · 2018-06-14T17:58:06Z

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

jenniferfell · 2018-06-14T17:58:57Z

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

jenniferfell · 2018-06-14T18:00:02Z

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

jenniferfell · 2018-06-14T19:44:43Z

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

mary-poppins · 2018-06-14T21:01:56Z

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

jbogarthyde · 2018-06-14T21:33:40Z

We are still testing the template, leaving title alone.

mary-poppins · 2018-06-14T21:50:37Z

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

mary-poppins · 2018-06-14T22:40:01Z

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

mary-poppins · 2018-06-15T16:09:33Z

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

vicb · 2018-06-16T10:25:02Z

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.

Not adressed

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

vicb · 2018-06-16T10:25:53Z

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

vicb · 2018-06-16T10:27:24Z

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

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.

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.

vicb · 2018-06-16T10:27:56Z

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

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).

vicb · 2018-06-16T10:28:27Z

not adressed.

vicb · 2018-06-19T08:08:08Z

vicb · 2018-06-19T08:10:01Z

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.

OK, got it.

vicb · 2018-06-19T08:12:00Z

in the _local_ time this was important too

vicb

Thanks for the updates.
A few more comments

mary-poppins · 2018-06-19T15:03:15Z

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

jbogarthyde · 2018-06-19T15:50:42Z

Addressed comments, and some more small fixes.

mary-poppins · 2018-06-19T15:59:01Z

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

mary-poppins · 2018-06-19T16:38:05Z

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

jbogarthyde · 2018-06-19T17:23:41Z

@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.

mhevery · 2018-06-21T20:12:02Z

http://test/OCL:201573473:BASE:201573500:1529611895746:6c6cb57

Comments addressed, Vic on vacation.

PR Close #24141

cexbrayat · 2018-06-25T17:56:12Z

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 · 2018-06-25T18:02:05Z

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

cexbrayat · 2018-06-25T18:13:13Z

@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.

angular-automatic-lock-bot · 2019-09-13T18:52:12Z

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.}

googlebot added the cla: yes label May 25, 2018

jbogarthyde added comp: docs labels May 25, 2018

jbogarthyde force-pushed the jb-apidoc-pipes branch from 33c65d1 to db133ab Compare June 13, 2018 18:42

jbogarthyde force-pushed the jb-apidoc-pipes branch from dd0d78c to 4b9b3c3 Compare June 14, 2018 15:15

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

jenniferfell approved these changes Jun 14, 2018

View reviewed changes

jenniferfell requested a review from vicb June 14, 2018 18:21

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

vicb reviewed Jun 16, 2018

View reviewed changes

vicb reviewed Jun 19, 2018

View reviewed changes

Comment thread packages/common/src/pipes/date_pipe.ts Outdated

vicb Jun 19, 2018

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

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

👍

vicb reviewed Jun 19, 2018

View reviewed changes

vicb previously requested changes Jun 19, 2018

View reviewed changes

jbogarthyde force-pushed the jb-apidoc-pipes branch from 3530aaa to bc59b64 Compare June 19, 2018 14:32

docs: test api doc for pipes

11c61a6

jbogarthyde force-pushed the jb-apidoc-pipes branch from bc59b64 to 11c61a6 Compare June 19, 2018 14:33

fixup! docs: test api doc for pipes

a4f7473

fixup! docs: test api doc for pipes

de33116

fixup! docs: test api doc for pipes

449373b

ocombe approved these changes Jun 20, 2018

View reviewed changes

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

jasonaden closed this in d244523 Jun 25, 2018

jasonaden pushed a commit that referenced this pull request Jun 25, 2018

docs: test api doc for pipes (#24141)

65744e4

PR Close #24141

cexbrayat mentioned this pull request Jun 25, 2018

docs: more info on currency digits #24661

Closed

3 tasks done

jbogarthyde deleted the jb-apidoc-pipes branch June 28, 2018 18:03

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 13, 2019

Conversation

jbogarthyde commented May 25, 2018

PR Checklist

PR Type

Does this PR introduce a breaking change?

Uh oh!

jbogarthyde commented May 25, 2018

Uh oh!

mary-poppins commented May 25, 2018

Uh oh!

mary-poppins commented May 25, 2018

Uh oh!

mary-poppins commented Jun 13, 2018

Uh oh!

mary-poppins commented Jun 13, 2018

Uh oh!

mary-poppins commented Jun 13, 2018

Uh oh!

mary-poppins commented Jun 14, 2018

Uh oh!

jenniferfell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jenniferfell commented Jun 14, 2018

Uh oh!

mary-poppins commented Jun 14, 2018

Uh oh!

jbogarthyde commented Jun 14, 2018

Uh oh!

mary-poppins commented Jun 14, 2018

Uh oh!

mary-poppins commented Jun 14, 2018

Uh oh!

mary-poppins commented Jun 15, 2018

Uh oh!

vicb Jun 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vicb Jun 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

vicb Jun 16, 2018 •

edited

Loading

vicb Jun 16, 2018 •

edited

Loading