GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — docs(aio): update architecture section by jbogarthyde · Pull Request #21569 · angular/angular

Conversation

@jbogarthyde

@jbogarthyde jbogarthyde commented Jan 16, 2018

Copy link
Copy Markdown
Contributor

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:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

Update doc with new section on architecture.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@googlebot googlebot added the cla: yes label Jan 16, 2018
@jbogarthyde

jbogarthyde commented Jan 16, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

@StephenFluin - The old PR was hopelessly munged. Please review this new PR with the changes we discussed, and some of the restored content that was lost in the previous PR.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gkalpak gkalpak added the comp: docs label Jan 16, 2018
@googlebot

googlebot commented Jan 16, 2018

Copy link
Copy Markdown

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@googlebot googlebot added cla: no and removed cla: yes labels Jan 16, 2018
@mary-poppins

mary-poppins commented Jan 17, 2018

Copy link
Copy Markdown

You can preview a324941 at https://pr21569-a324941.ngbuilds.io/.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mary-poppins

mary-poppins commented Jan 17, 2018

Copy link
Copy Markdown

You can preview d720ba1 at https://pr21569-d720ba1.ngbuilds.io/.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
StephenFluin
StephenFluin approved these changes Jan 25, 2018
View reviewed changes
@jenniferfell
jenniferfell self-requested a review January 25, 2018 22:48
@jenniferfell jenniferfell assigned jbogarthyde Jan 25, 2018
@gkalpak gkalpak added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews target: patch This PR is targeted for the next patch release labels Jan 26, 2018
@gkalpak

gkalpak commented Jan 26, 2018

Copy link
Copy Markdown
Member

@jbogarthyde, can you please squash all commits into one and rebase on master?
jbogarthyde reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jbogarthyde
jbogarthyde force-pushed the docs_architecture branch from d720ba1 to fdc0d52 Compare January 26, 2018 17:12
@mary-poppins

mary-poppins commented Jan 26, 2018

Copy link
Copy Markdown

You can preview fdc0d52 at https://pr21569-fdc0d52.ngbuilds.io/.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gkalpak gkalpak added cla: yes and removed cla: no labels Jan 26, 2018
@jenniferfell

jenniferfell commented Feb 1, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

(wrong PR)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jenniferfell jenniferfell mentioned this pull request Feb 2, 2018
docs(architecture): Change hero-detail reference to app-hero-detail #21957
Closed
2 tasks
@jenniferfell

jenniferfell commented Feb 2, 2018

Copy link
Copy Markdown
Contributor

I believe the rewrites in this PR also fix #21957 and #21955
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ngbot

ngbot Bot commented Feb 5, 2018

Copy link
Copy Markdown

Hi @jbogarthyde! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
1 similar comment
@ngbot

ngbot Bot commented Feb 5, 2018

Copy link
Copy Markdown

Hi @jbogarthyde! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jbogarthyde
jbogarthyde force-pushed the docs_architecture branch from fdc0d52 to 4e9e77f Compare February 6, 2018 16:11
@mary-poppins

mary-poppins commented Feb 6, 2018

Copy link
Copy Markdown

You can preview 4e9e77f at https://pr21569-4e9e77f.ngbuilds.io/.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mary-poppins

mary-poppins commented Feb 6, 2018

Copy link
Copy Markdown

You can preview ffa43f0 at https://pr21569-ffa43f0.ngbuilds.io/.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@wimgoeman

wimgoeman commented Feb 8, 2018

Copy link
Copy Markdown

I don't think #21957 is fixed with this. aio/content/guide/architecture-components.md still references <hero-detail> while the html in aio/content/examples/architecture/src/app/hero-list.component.html references <app-hero-detail>.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mary-poppins

mary-poppins commented Feb 8, 2018

Copy link
Copy Markdown

You can preview 4ffe4bd at https://pr21569-4ffe4bd.ngbuilds.io/.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jenniferfell jenniferfell mentioned this pull request Feb 15, 2018
Documentation error which says NgModule instead of BrowserModule #22158
Closed
@jenniferfell

jenniferfell commented Feb 15, 2018

Copy link
Copy Markdown
Contributor

This will also close issue #22158
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jbogarthyde
jbogarthyde force-pushed the docs_architecture branch from 5d7965b to 7ccf9ac Compare March 13, 2018 19:47
@mary-poppins

mary-poppins commented Mar 13, 2018

Copy link
Copy Markdown

You can preview 7ccf9ac at https://pr21569-7ccf9ac.ngbuilds.io/.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jbogarthyde
jbogarthyde force-pushed the docs_architecture branch from 7ccf9ac to 784d54a Compare March 14, 2018 15:17
@mary-poppins

mary-poppins commented Mar 14, 2018

Copy link
Copy Markdown

You can preview 784d54a at https://pr21569-784d54a.ngbuilds.io/.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jenniferfell jenniferfell added aio: preview and removed aio: preview labels Mar 14, 2018
@jenniferfell
jenniferfell requested a review from StephenFluin March 14, 2018 17:26
StephenFluin
StephenFluin approved these changes Mar 14, 2018
View reviewed changes
gkalpak
gkalpak suggested changes Mar 14, 2018
View reviewed changes
Comment thread aio/content/guide/architecture-services.md Outdated

@gkalpak gkalpak Mar 14, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Saying that it "uses a Promise to fetch" sounds weird.
It uses the Backend service, which in turns uses something else under the hood (e.g. XMLHttpRequest, fetch, etc.) and returns a Promise to represent the future result/response.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jbogarthyde jbogarthyde Mar 14, 2018

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread aio/content/guide/architecture-services.md Outdated

@gkalpak gkalpak Mar 14, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This section makes it sound a little bit as if DI only works on components, while it also works (and is extensively used) in other places, such as services, pipes, NgModules.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jbogarthyde jbogarthyde Mar 14, 2018

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread aio/content/guide/architecture-modules.md Outdated

@gkalpak gkalpak Mar 14, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

they become accessible in all parts of the app

Not true for lazy-loaded modules, which get their own injector 😁
But maybe this simplification is fine for this introductory guide.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jbogarthyde jbogarthyde Mar 14, 2018

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

added note that you can specify providers at component level (which is now preferred, since it makes them tree-shakeable).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread aio/content/guide/architecture-modules.md Outdated

@gkalpak gkalpak Mar 14, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

because other components --> because other modules

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jbogarthyde jbogarthyde Mar 14, 2018

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread aio/content/guide/architecture-modules.md Outdated

@gkalpak gkalpak Mar 14, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

An NgModule has a root component, created during bootstrap

This mostly applies to the root NgModule only.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jbogarthyde jbogarthyde Mar 14, 2018

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Does this work?
NgModules provide a compilation context for their components. A root NgModule always has a root component that is created during bootstrap, but any NgModule can include any number of additional components, which can be loaded through the router or created through the template. The components that belong to an NgModule share a compilation context.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gkalpak reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread aio/content/guide/architecture-components.md Outdated

@gkalpak gkalpak Mar 14, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Calling a "custom element" is confusing (since people might think of https://developer.mozilla.org/en-US/docs/Web/Web_Components/Using_custom_elements).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jbogarthyde jbogarthyde Mar 14, 2018

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread aio/content/guide/architecture-components.md Outdated

@gkalpak gkalpak Mar 14, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

which alters the HTML before the view is rendered

It can modify the HTML at any time, not just before the view is rendered.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jbogarthyde jbogarthyde Mar 14, 2018

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread aio/content/guide/architecture-components.md Outdated

@gkalpak gkalpak Mar 14, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Why not link to https://pr21569-784d54a.ngbuilds.io/guide/user-input#binding-to-user-input-events instead?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jbogarthyde jbogarthyde Mar 14, 2018

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread aio/content/guide/architecture-components.md Outdated

@gkalpak gkalpak Mar 14, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

using the ngModel directive

This makes it sound as if two-way data binding is using (or depends on) ngModel, which is not true. Two-way data binding has nothing to do with ngModel, except for the fact that two-way data binding is often used to bind to ngModel.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jbogarthyde jbogarthyde Mar 14, 2018

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread aio/content/guide/architecture-components.md Outdated

@gkalpak gkalpak Mar 14, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Might also be worth mentioning (or showing in the example code-snippet above) that pipes can also take arguments.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jbogarthyde jbogarthyde Mar 14, 2018

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Can you provide an example?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@gkalpak gkalpak Mar 14, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

https://angular.io/api/common/DatePipe#examples

(I.e. {{ interpolated_value | pipe_name : arg1 : arg2 }})

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mary-poppins

mary-poppins commented Mar 14, 2018

Copy link
Copy Markdown

You can preview 96e6f45 at https://pr21569-96e6f45.ngbuilds.io/.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jbogarthyde
docs(aio): update architecture section
371102b
@jbogarthyde
jbogarthyde force-pushed the docs_architecture branch from 96e6f45 to ac2f558 Compare March 15, 2018 15:17
@mary-poppins

mary-poppins commented Mar 15, 2018

Copy link
Copy Markdown

You can preview ac2f558 at https://pr21569-ac2f558.ngbuilds.io/.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mary-poppins

mary-poppins commented Mar 15, 2018

Copy link
Copy Markdown

You can preview 503647f at https://pr21569-503647f.ngbuilds.io/.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gkalpak
gkalpak approved these changes Mar 15, 2018
View reviewed changes

@gkalpak gkalpak left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

A couple of minor comments.
Also the commits should be squashed together.

LGTM otherwise.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@jbogarthyde
docs: incorporate suggestions and corrections from gkalpak
591b9a7
@jbogarthyde
jbogarthyde force-pushed the docs_architecture branch from 503647f to 591b9a7 Compare March 15, 2018 17:47
@mary-poppins

mary-poppins commented Mar 15, 2018

Copy link
Copy Markdown

You can preview 591b9a7 at https://pr21569-591b9a7.ngbuilds.io/.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jenniferfell jenniferfell added the action: merge The PR is ready for merge by the caretaker label Mar 15, 2018
@jenniferfell

jenniferfell commented Mar 15, 2018

Copy link
Copy Markdown
Contributor

Okay..finally passing all checks. Don't touch anything. :-) Merge merge quick. :-)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mhevery pushed a commit that referenced this pull request Mar 15, 2018
@jbogarthyde @mhevery
docs(aio): update architecture section (#21569)
e1b82a0 
PR Close #21569
mhevery pushed a commit that referenced this pull request Mar 15, 2018
@jbogarthyde @mhevery
docs: incorporate suggestions and corrections from gkalpak (#21569)
2731eca 
PR Close #21569
@mhevery mhevery closed this in 04ca77e Mar 15, 2018
mhevery pushed a commit that referenced this pull request Mar 15, 2018
@jbogarthyde @mhevery
docs: incorporate suggestions and corrections from gkalpak (#21569)
02e6ac2 
PR Close #21569
@jenniferfell jenniferfell mentioned this pull request Apr 25, 2018
Architecture guide references 'app-hero-detail' tag as 'hero-detail' #21955
Closed
@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Sep 13, 2019

Copy link
Copy Markdown

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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.

Reviewers

3 more reviewers
@StephenFluin StephenFluin StephenFluin approved these changes
@gkalpak gkalpak gkalpak approved these changes
@jenniferfell jenniferfell jenniferfell approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@jbogarthyde jbogarthyde

Labels

action: merge The PR is ready for merge by the caretaker cla: yes target: patch This PR is targeted for the next patch release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@jbogarthyde @googlebot @mary-poppins @gkalpak @jenniferfell @wimgoeman @StephenFluin
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.