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.

Sign up for GitHub

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

Jump to bottom

docs: add new getting started preview #27684

Closed
wants to merge 3 commits into from
Closed

docs: add new getting started preview #27684

wants to merge 3 commits into from

Conversation

brandonroberts
Copy link
Contributor

@brandonroberts brandonroberts commented Dec 14, 2018 •
edited by StephenFluin

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
  • 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
  • No

Other information

For preview generation purposes only at the moment

@googlebot
Copy link

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 or co-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 all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

@mary-poppins
Copy link

You can preview 3e0b34a at https://pr27684-3e0b34a.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 0095ccf at https://pr27684-0095ccf.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 09ff763 at https://pr27684-09ff763.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 1192bf1 at https://pr27684-1192bf1.ngbuilds.io/.

@mary-poppins
Copy link

You can preview c9201a4 at https://pr27684-c9201a4.ngbuilds.io/.

@mary-poppins
Copy link

You can preview efffeb1 at https://pr27684-efffeb1.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 7fdfe69 at https://pr27684-7fdfe69.ngbuilds.io/.

trotyl
trotyl reviewed Jan 4, 2019
View reviewed changes
aio/content/tutorial/getting-started-architecture.md Outdated Show resolved Hide resolved
aio/content/tutorial/getting-started-data.md Outdated Show resolved Hide resolved
aio/content/tutorial/getting-started-data.md Outdated Show resolved Hide resolved
aio/content/tutorial/getting-started-data.md Outdated Show resolved Hide resolved
aio/content/tutorial/getting-started-data.md Outdated Show resolved Hide resolved
aio/content/tutorial/getting-started-data.md Outdated Show resolved Hide resolved
aio/content/tutorial/getting-started-data.md Outdated Show resolved Hide resolved
aio/content/tutorial/getting-started-data.md Outdated Show resolved Hide resolved
aio/content/tutorial/getting-started-data.md Outdated Show resolved Hide resolved
aio/content/tutorial/getting-started-data.md Outdated Show resolved Hide resolved
@brandonroberts brandonroberts requested review from IgorMinar and a team as code owners January 9, 2019 03:08
@mary-poppins
Copy link

You can preview 60d5016 at https://pr27684-60d5016.ngbuilds.io/.

@mary-poppins
Copy link

You can preview e171424 at https://pr27684-e171424.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 8a7585e at https://pr27684-8a7585e.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 97b7d8c at https://pr27684-97b7d8c.ngbuilds.io/.

@ngbot ngbot bot added this to the needsTriage milestone Jan 24, 2019
@brandonroberts brandonroberts requested a review from a team as a code owner January 30, 2019 17:49
@mary-poppins
Copy link

You can preview 1656cee at https://pr27684-1656cee.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 9b1a876 at https://pr27684-9b1a876.ngbuilds.io/.

@mary-poppins
Copy link

You can preview ed53be3 at https://pr27684-ed53be3.ngbuilds.io/.

jenniferfell
jenniferfell approved these changes Mar 28, 2019
View reviewed changes
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.

A fox-in-the-henhouse approval

@mary-poppins
Copy link

You can preview 0b01f06 at https://pr27684-0b01f06.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 8381120 at https://pr27684-8381120.ngbuilds.io/.

gkalpak
gkalpak requested changes Mar 28, 2019
View reviewed changes
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

Mostly LGTM. A couple of questions/comments:

  • I don't think aio/content/examples/.DS_Store should be checked in.
  • Are all the checked in images used?
  • Why so many numbered lists? 😁

aio/content/examples/getting-started-v0/src/app/product-list/product-list.component.ts Show resolved Hide resolved
aio/content/examples/getting-started/e2e/src/app.e2e-spec.ts Show resolved Hide resolved
aio/content/examples/getting-started/e2e/src/app.e2e-spec.ts Outdated Show resolved Hide resolved
aio/content/examples/getting-started/e2e/src/app.e2e-spec.ts Outdated Show resolved Hide resolved
aio/content/examples/getting-started/e2e/src/app.e2e-spec.ts Show resolved Hide resolved
aio/content/getting-started/data.md Outdated Show resolved Hide resolved
aio/content/getting-started/forms.md Outdated Show resolved Hide resolved
aio/content/getting-started/forms.md Show resolved Hide resolved
aio/content/getting-started/forms.md Show resolved Hide resolved
aio/content/getting-started/forms.md Outdated Show resolved Hide resolved
@gkalpak
Copy link
Member

gkalpak commented Mar 28, 2019

Oh, and the new guides should have some CODEOWNERS (e.g. fw-docs-intro).

@mary-poppins
Copy link

You can preview 3909d6d at https://pr27684-3909d6d.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 35f90f2 at https://pr27684-35f90f2.ngbuilds.io/.

@brandonroberts brandonroberts added the action: merge The PR is ready for merge by the caretaker label Mar 29, 2019
gkalpak
gkalpak approved these changes Mar 29, 2019
View reviewed changes
Copy link
Member

@gkalpak gkalpak left a comment •
edited

Choose a reason for hiding this comment

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

I left a couple of comment, but otherwise LGTM (for docs-infra) 💪
Feel free to land this as is and address my comments in a follow-up PR.

aio/content/guide/prerequisites-setup.md
@@ -136,8 +132,6 @@ After the Tutorial and Architecture guide, you'll be ready to continue exploring

Angular assumes specific versions of many related technologies and tools, such as TypeScript, Karma, Protractor, tsickle, zone.js.
Copy link
Member

Choose a reason for hiding this comment

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

Sorry I missed it on the first pass, but I don't think Angular assumes specific versions of all these things. For example:

  • I don't think Angular cares about Karma or Protractor versions.
  • It might care about tsickle, but only if you use tsickle which most people won't (especially beginners).
  • No mention of RxJS? 😱

The compatible version ranges for these packages can be found in the peerDependencies fields in the package.json files of the published Angular packages. For example:

  • node_modules/@angular/core/package.json > peerDependencies contains the required version ranges for RxJS and Zone.js.
  • node_modules/@angular/compiler-cli/package.json > peerDependencies contains the required version range for TypeScript.

aio/content/guide/prerequisites-setup.md
@@ -136,8 +132,6 @@ After the Tutorial and Architecture guide, you'll be ready to continue exploring

Angular assumes specific versions of many related technologies and tools, such as TypeScript, Karma, Protractor, tsickle, zone.js.

For details, see the [master `package.json` file](https://github.com/angular/angular/blob/master/package.json) for Angular.

The `package.json` is organized into two groups of packages:
Copy link
Member

Choose a reason for hiding this comment

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

Now that the reference to package.json is removed, this sentence is out of context.
Consider mentioning the package.json files of the installed Angular packages (see my previous comment) and then talk about the peerDependencies (which are the only kind of dependencies that should be relevant to app developers).

@brandonroberts
Copy link
Contributor Author

@gkalpak Thanks. We will address the remaining items in a follow-up PR.

@jasonaden jasonaden added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Mar 29, 2019
@jasonaden
Copy link
Contributor

Not showing as green due to code review, but it has the reviews it needs. Merging.

@jasonaden jasonaden closed this in 12c9bd2 Mar 29, 2019
@gkalpak
Copy link
Member

gkalpak commented Mar 29, 2019 •
edited

Well, it didn't have the reviews it needed: .github/CODEOWNERS can only be approved by Igor or @angular/framework-global-approvers, none of which approved the PR.
But I suspect it's fine, since the change was non-controversial:tm: :grin:

cexbrayat added a commit to cexbrayat/angular that referenced this pull request Apr 2, 2019
@cexbrayat
docs: fix typos in getting-started
c6d6b4f 
PR angular#27684 introduced a new getting-started guide and a few typos slipped through the review.
@cexbrayat cexbrayat mentioned this pull request Apr 2, 2019
Closed
14 tasks
jasonaden pushed a commit that referenced this pull request Apr 2, 2019
@cexbrayat @jasonaden
docs: fix typos in getting-started (#29665)
9e4c0bd 
PR #27684 introduced a new getting-started guide and a few typos slipped through the review.

PR Close #29665
DeveloperFromUkraine pushed a commit to DeveloperFromUkraine/angular that referenced this pull request Apr 11, 2019
@cexbrayat @DeveloperFromUkraine
docs: fix typos in getting-started (angular#29665)
104e28d 
PR angular#27684 introduced a new getting-started guide and a few typos slipped through the review.

PR Close angular#29665
wKoza pushed a commit to wKoza/angular that referenced this pull request Apr 17, 2019
@brandonroberts @wKoza
docs: add new getting started guide (angular#27684)
c29112d 
PR Close angular#27684
wKoza pushed a commit to wKoza/angular that referenced this pull request Apr 17, 2019
@cexbrayat @wKoza
docs: fix typos in getting-started (angular#29665)
54e23c0 
PR angular#27684 introduced a new getting-started guide and a few typos slipped through the review.

PR Close angular#29665
@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 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@trotyl trotyl trotyl left review comments

@sreevani-ship-it sreevani-ship-it sreevani-ship-it left review comments

@StephenFluin StephenFluin StephenFluin approved these changes

@gkalpak gkalpak gkalpak approved these changes

@jenniferfell jenniferfell jenniferfell approved these changes

@IgorMinar IgorMinar Awaiting requested review from IgorMinar

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker aio: preview cla: yes effort3: weeks feature Issue that requests a new feature merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note risk: medium target: major This PR is targeted for the next major release
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

None yet