Skip to content

Conversation

@ande1005
Copy link
Contributor

The 'Managing Data' part of the tutorial is missing an instruction to import Product into cart.service.ts

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?

The angular tutorial does not work as currently written. Cannot find name 'Product' error is thrown when creating the cart.service.ts file.

Issue Number: N/A

What is the new behavior?

Include an instruction to import Product from './products'

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@pullapprove pullapprove bot requested a review from aikithoughts June 29, 2021 15:06
@google-cla google-cla bot added the cla: yes label Jun 29, 2021
Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

Thanks @ande1005 - I think we could also update the "props" docregion in the example code to show the import too: around this line https://github.com/angular/angular/blob/master/aio/content/examples/getting-started/src/app/cart.service.ts#L5

Add docregion markers so it ends up looking like:

// #docregion props
import { Product } from './products';
// #enddocregion props, import-http

@mary-poppins
Copy link

You can preview 9b0dd92 at https://pr42701-9b0dd92.ngbuilds.io/.
You can preview f0d805b at https://pr42701-f0d805b.ngbuilds.io/.

@petebacondarwin petebacondarwin changed the title docs(docs-infra): add missing import to cart service instructions docs: add missing import to cart service instructions Jun 29, 2021
Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

This is looking much better. I think we might need to remove the // #docplaster comment so that we get nice ... between the import statements and the class in the snippet.

Screenshot 2021-06-29 at 19 43 01

Also, you'll note that the CI lint job is failing because the commits do not follow our required syntax. On your next update, can you squash all the commits into one and give it a commit message of something like:

docs: add missing import to `CartService` tutorial instructions

@ande1005 ande1005 changed the title docs: add missing import to cart service instructions docs: add missing import to CartService tutorial instructions Jul 1, 2021
@google-cla
Copy link

google-cla bot commented Jul 1, 2021

All (the pull request submitter and all commit authors) CLAs are signed, but 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 by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@petebacondarwin
Copy link
Contributor

Hi @ande1005 - thanks for making the changes and squashing the commit.
Unfortunately we need to rebase the commit on top of master to get the CI to run (including the valuable AIO preview).
Also it looks like you might have used a different email address in git (when you did the previous rebase) than that used to log into GitHub? Can you check that please.

@mary-poppins
Copy link

You can preview 25700dd at https://pr42701-25700dd.ngbuilds.io/.

Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

Reviewed-for: global-docs-approvers

@petebacondarwin petebacondarwin removed the request for review from aikithoughts July 7, 2021 07:53
@petebacondarwin petebacondarwin added action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Jul 7, 2021
@atscott atscott closed this in e064f17 Jul 7, 2021
@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 Aug 7, 2021
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 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.

4 participants