docs: NgModule guide prose for CLI (partial) #19776

wardbell · 2017-10-18T07:46:50Z

See Issue #19510

BEGINS necessary changes for NgModules guide ... before I realized that the changes that @kapunahelewong is making have not yet been merged! Handing this off to her.

Update 10/20

@kapunahelewong says we should finish this PR because her substantial changes to NgModules guide will not be ready soon enough. She will incorporate the changes in this PR into her work at her future convenience.

Update 10/23

I think it's "done". At least it's done enough to count as converted for CLI.

Update 10/25

Also replaces “Angular Module” with “NgModule” wherever that is clarifying.
Continue using “module” when qualified as in “feature module”, “root module”, “routing module”, etc.

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:

mary-poppins · 2017-10-22T05:39:40Z

You can preview 40290ab at https://pr19776-40290ab.ngbuilds.io/.

mary-poppins · 2017-10-22T05:57:49Z

You can preview c24bb83 at https://pr19776-c24bb83.ngbuilds.io/.

mary-poppins · 2017-10-22T07:04:02Z

You can preview 596e941 at https://pr19776-596e941.ngbuilds.io/.

mary-poppins · 2017-10-22T17:47:20Z

You can preview 92d857c at https://pr19776-92d857c.ngbuilds.io/.

mary-poppins · 2017-10-23T00:40:42Z

You can preview a1f1299 at https://pr19776-a1f1299.ngbuilds.io/.

skatterwe · 2017-10-23T02:02:37Z

Ich kehre zurück am 01.11.2017. Ich werde Ihre Nachricht nach meiner Rückkehr beantworten. In dringenden Fällen senden Sie bitte eine Kopie Ihrer E-Mail für technische Angelegenheiten an entwicklung@arxes-tolina.de, ansonsten an info@arxes-tolina.de. Ein anderer Mitarbeiter wird sich dann Ihrer E-Mail annehmen. Hinweis: Dies ist eine automatische Antwort auf Ihre Nachricht "Re: [angular/angular] docs: NgModule guide prose for CLI (partial) (#19776)" gesendet am 23.10.2017 02:41:18. Diese ist die einzige Benachrichtigung, die Sie empfangen werden, während diese Person abwesend ist.

mary-poppins · 2017-10-23T10:22:59Z

You can preview cb8631e at https://pr19776-cb8631e.ngbuilds.io/.

mary-poppins · 2017-10-23T10:49:57Z

You can preview ed10b8f at https://pr19776-ed10b8f.ngbuilds.io/.

skatterwe · 2017-10-23T11:03:03Z

Ich kehre zurück am 01.11.2017. Ich werde Ihre Nachricht nach meiner Rückkehr beantworten. In dringenden Fällen senden Sie bitte eine Kopie Ihrer E-Mail für technische Angelegenheiten an entwicklung@arxes-tolina.de, ansonsten an info@arxes-tolina.de. Ein anderer Mitarbeiter wird sich dann Ihrer E-Mail annehmen. Hinweis: Dies ist eine automatische Antwort auf Ihre Nachricht "Re: [angular/angular] docs: NgModule guide prose for CLI (partial) (#19776)" gesendet am 23.10.2017 12:23:49. Diese ist die einzige Benachrichtigung, die Sie empfangen werden, während diese Person abwesend ist.

mary-poppins · 2017-10-25T19:59:59Z

You can preview 6e59283 at https://pr19776-6e59283.ngbuilds.io/.

mary-poppins · 2017-11-16T07:11:38Z

You can preview 7381a36 at https://pr19776-7381a36.ngbuilds.io/.

mary-poppins · 2017-11-16T08:02:55Z

You can preview 9a9f9ab at https://pr19776-9a9f9ab.ngbuilds.io/.

wardbell · 2017-11-16T08:24:52Z

@gkalpak Not sure why but apparently this PR needs your OK. It's part of the samples->CLI project.

petebacondarwin

Nice piece of work @wardbell. I have approved it with a handful (~40!) minor comments, from which you can pick and choose before labelling for merge.

petebacondarwin · 2017-11-17T09:15:19Z

aio/content/examples/ngmodule/src/app/app-routing.module.3.ts

@@ -1,14 +1,19 @@
 import { NgModule } from '@angular/core';
 import { Routes, RouterModule } from '@angular/router';

-export const routes: Routes = [
+import { ContactModule }      from './contact/contact.module.3';


unusual whitespace?

petebacondarwin · 2017-11-17T09:20:28Z

aio/content/examples/ngmodule/src/app/app.module.1b.ts

-
-import { FormsModule }        from '@angular/forms';
+import { ContactService }     from './contact/contact.service';
+import { ContactHighlightDirective } from './contact/contact-highlight.directive';


is it a conscious decision to align the froms unless the import is too wide?

I was aligning imports for readability for a long time. But it's difficult to maintain and I'm abandoning that effort as I go. In this PR, due to time/money constraints, I'll limit adjustments to the worst cases that you've identified.

petebacondarwin · 2017-11-17T09:22:55Z

aio/content/examples/ngmodule/src/app/contact/contact-highlight.directive.ts

@@ -1,4 +1,4 @@
-/* tslint:disable */
+// #docplaster


not related to this PR but we should get agreement from "someone" to change the default doc-plaster to the empty string :-)

I lost that battle so long ago ... as in on DAY ONE. You're welcome to take it up again.

petebacondarwin · 2017-11-17T09:24:59Z

aio/content/examples/ngmodule/src/app/contact/contact.component.html

-      [(ngModel)]="contact.name"
-        name="name"  #name="ngModel" >
+    [(ngModel)]="contact.name"
+    name="name"  #name="ngModel" >


should these attributes be indented or is this a conscious coding style in this file?

Indented. Something messed it up. Fixing.

petebacondarwin · 2017-11-17T09:28:08Z

aio/content/examples/ngmodule/src/app/contact/contact.service.ts

+export class ContactService implements OnDestroy {
+// #enddocregion
+  constructor() { console.log('ContactService instance created.'); }
+  ngOnDestroy() { console.log('ContactService instance destroyed.'); }


Are these just here for your debugging or important parts of the example?

Ahah! I see you mention them here: https://github.com/angular/angular/pull/19776/files#diff-ed3e5f8edaa8c7b9920b0579fa7afe51R1131

petebacondarwin · 2017-11-17T11:05:16Z

aio/content/guide/ngmodule.md


 <div class="l-sub-section">

-
-
 A lazy-loaded module location is a _string_, not a _type_.


The references to "module" in this paragraph need changing to NgModule.

petebacondarwin · 2017-11-17T11:10:48Z

aio/content/guide/ngmodule.md

 </code-example>

+Re-exporting  `RouterModule` makes the router directives


superfluous space

petebacondarwin · 2017-11-17T11:24:29Z

aio/content/guide/ngmodule.md

-      highlight.directive.ts
-    </div>
+Do not import the `HeroModule` or `CrisisModule` or any of their classes outside of their respective file folders.
+If you do, you will unintentionally load those modules and all of their code,


unintentionally "eager" load?

petebacondarwin · 2017-11-17T12:30:13Z

aio/content/guide/ngmodule.md

 <div class="alert is-critical">

-
-
 Do *not* specify app-wide singleton `providers` in a shared module.
 A lazy-loaded module that imports that shared module makes its own copy of the service.


lazy-loaded NgModule

petebacondarwin · 2017-11-17T12:30:37Z

aio/content/guide/ngmodule.md

 The `@NgModule` metadata should be familiar.
-You declare the `TitleComponent`  because this module owns it and you export it
-because `AppComponent` (which is in `AppModule`) displays the title in its template.
+You declare the `TitleComponent`  because this module owns it. 


module -> NgModule

Also replaces “Angular Module” with “NgModule” wherever that is clarifying. Continue using “module” when qualified as in “feature module”, “root module”, “routing module”, etc.

wardbell · 2017-11-17T19:18:46Z

@petebacondarwin Adopted almost all of your most-welcome suggestions. Will add the merge label as soon as it clears CI. Thanks again!

mary-poppins · 2017-11-17T19:32:30Z

You can preview 02f40d3 at https://pr19776-02f40d3.ngbuilds.io/.

wardbell · 2017-11-20T16:29:53Z

The CI fail is a flake - a SauceLabs timeout - that has nothing to do with this PR.

mhevery · 2017-11-22T16:44:35Z

@wardbell all docs changes should go to both master and patch, please label accordingly next time.

Also replaces “Angular Module” with “NgModule” wherever that is clarifying. Continue using “module” when qualified as in “feature module”, “root module”, “routing module”, etc. PR Close #19776

petebacondarwin · 2017-11-22T16:55:37Z

@wardbell all docs changes should go to both master and patch, please label accordingly next time.

@mhevery - that may not be the case! If the docs change refers to something that is only on master then it would not make sense to land it on patch. I accept that in this case @wardbell may have forgotten that patch now refers to 5.x (whereas recently it referred to 4.x) so it is very likely that this is supposed to land on both.

wardbell · 2017-11-22T19:19:45Z

@petebacondarwin @mhevery Yes, I thought patch was still 4.x.

angular-automatic-lock-bot · 2019-09-12T22:20:59Z

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

wardbell added comp: docs labels Oct 18, 2017

wardbell self-assigned this Oct 18, 2017

wardbell requested review from Foxandxss and kapunahelewong October 18, 2017 07:46

googlebot added the cla: yes label Oct 18, 2017

wardbell force-pushed the cli-ngmodule branch 2 times, most recently from 201191b to 4acc5f3 Compare October 18, 2017 07:59

wardbell changed the title ~~docs: Bootstrapping and NgModule prose for CLI~~ docs: NgModule prose for CLI (partial) Oct 18, 2017

wardbell force-pushed the cli-ngmodule branch from 4acc5f3 to dfd1f60 Compare October 18, 2017 08:01

wardbell changed the title ~~docs: NgModule prose for CLI (partial)~~ docs: NgModule guide prose for CLI (partial) Oct 18, 2017

Foxandxss mentioned this pull request Oct 18, 2017

List of updates needed for CLI docs #19510

Closed

24 tasks

wardbell force-pushed the cli-ngmodule branch from dfd1f60 to 40290ab Compare October 22, 2017 05:28

wardbell force-pushed the cli-ngmodule branch from 40290ab to c24bb83 Compare October 22, 2017 05:40

wardbell force-pushed the cli-ngmodule branch from c24bb83 to 596e941 Compare October 22, 2017 06:49

wardbell force-pushed the cli-ngmodule branch from 596e941 to 92d857c Compare October 22, 2017 17:11

wardbell force-pushed the cli-ngmodule branch from 92d857c to a1f1299 Compare October 23, 2017 00:28

wardbell force-pushed the cli-ngmodule branch 2 times, most recently from c9d8ecc to cb8631e Compare October 23, 2017 10:10

wardbell force-pushed the cli-ngmodule branch from cb8631e to ed10b8f Compare October 23, 2017 10:35

wardbell force-pushed the cli-ngmodule branch from ed10b8f to c7c5f1f Compare October 23, 2017 11:00

wardbell force-pushed the cli-ngmodule branch from 348de03 to 6e59283 Compare October 25, 2017 19:41

wardbell force-pushed the cli-ngmodule branch 2 times, most recently from cc3c492 to 7381a36 Compare November 16, 2017 07:02

wardbell force-pushed the cli-ngmodule branch from 7381a36 to 9a9f9ab Compare November 16, 2017 07:50

wardbell requested a review from gkalpak November 16, 2017 08:24

wardbell added the target: major This PR is targeted for the next major release label Nov 16, 2017

petebacondarwin approved these changes Nov 17, 2017

View reviewed changes

petebacondarwin added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Nov 17, 2017

docs: NgModule guide prose for CLI (partial)

02f40d3

Also replaces “Angular Module” with “NgModule” wherever that is clarifying. Continue using “module” when qualified as in “feature module”, “root module”, “routing module”, etc.

wardbell force-pushed the cli-ngmodule branch from 9a9f9ab to 02f40d3 Compare November 17, 2017 19:16

wardbell added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Nov 20, 2017

mhevery closed this in 816d5ba Nov 21, 2017

mhevery added target: patch This PR is targeted for the next patch release and removed target: major This PR is targeted for the next major release labels Nov 22, 2017

Foxandxss deleted the cli-ngmodule branch November 22, 2017 17:07

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


		<div class="l-sub-section">



		A lazy-loaded module location is a _string_, not a _type_.

		</code-example>

		Re-exporting `RouterModule` makes the router directives

docs: NgModule guide prose for CLI (partial) #19776

docs: NgModule guide prose for CLI (partial) #19776

Conversation

wardbell commented Oct 18, 2017 • edited Loading

Update 10/20

Update 10/23

Update 10/25

PR Checklist

PR Type

mary-poppins commented Oct 22, 2017

mary-poppins commented Oct 22, 2017

mary-poppins commented Oct 22, 2017

mary-poppins commented Oct 22, 2017

mary-poppins commented Oct 23, 2017

skatterwe commented Oct 23, 2017 via email

mary-poppins commented Oct 23, 2017

mary-poppins commented Oct 23, 2017

skatterwe commented Oct 23, 2017 via email

mary-poppins commented Oct 25, 2017

mary-poppins commented Nov 16, 2017

mary-poppins commented Nov 16, 2017

wardbell commented Nov 16, 2017 • edited Loading

petebacondarwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wardbell Nov 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wardbell commented Nov 17, 2017

mary-poppins commented Nov 17, 2017

wardbell commented Nov 20, 2017

mhevery commented Nov 22, 2017

petebacondarwin commented Nov 22, 2017

wardbell commented Nov 22, 2017

angular-automatic-lock-bot bot commented Sep 12, 2019

wardbell commented Oct 18, 2017 •

edited

Loading

wardbell commented Nov 16, 2017 •

edited

Loading

wardbell Nov 17, 2017 •

edited

Loading