Skip to content

docs(aio): update ToH for CLI #19811

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.

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

Merged
merged 1 commit into from
Nov 6, 2017
Merged

docs(aio): update ToH for CLI #19811

merged 1 commit into from
Nov 6, 2017

Conversation

Foxandxss
Copy link
Member

@Foxandxss Foxandxss commented Oct 19, 2017

Ready to go

@mary-poppins
Copy link

You can preview e45926b at https://pr19811-e45926b.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 9de6a6a at https://pr19811-9de6a6a.ngbuilds.io/.

@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 by someone other than the pull request submitter. We need to confirm that they're okay 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.

@mary-poppins
Copy link

You can preview 88ff3bf at https://pr19811-88ff3bf.ngbuilds.io/.

@mary-poppins
Copy link

You can preview bc3d17d at https://pr19811-bc3d17d.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 8627274 at https://pr19811-8627274.ngbuilds.io/.

@wardbell wardbell force-pushed the aio-cli-toh branch 2 times, most recently from 0de98f9 to fb5b4a9 Compare October 25, 2017 05:25
@mary-poppins
Copy link

You can preview fb5b4a9 at https://pr19811-fb5b4a9.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 6e8784f at https://pr19811-6e8784f.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 8dbfb1b at https://pr19811-8dbfb1b.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 93c0096 at https://pr19811-93c0096.ngbuilds.io/.

@mary-poppins
Copy link

You can preview cc444b5 at https://pr19811-cc444b5.ngbuilds.io/.

@mary-poppins
Copy link

You can preview ae23674 at https://pr19811-ae23674.ngbuilds.io/.

@mary-poppins
Copy link

You can preview d302883 at https://pr19811-d302883.ngbuilds.io/.

@mary-poppins
Copy link

You can preview d1db616 at https://pr19811-d1db616.ngbuilds.io/.

@wardbell wardbell force-pushed the aio-cli-toh branch 2 times, most recently from fd8e1b6 to 09d23aa Compare October 27, 2017 02:22
@Nicolas-Bouteille
Copy link

Ok I see!

@Nicolas-Bouteille
Copy link

  1. HTTP >> Add HeroService.addHero()
    TSLint reports something I don't understand:
    shadowed name
    Also, what's with the 'w/' ? Does it mean something or is it a typo?

@Foxandxss
Copy link
Member Author

w/ is an english shorthand which means "with", there is also w/o which is without. We should remove that I guess.

@Nicolas-Bouteille
Copy link

ok thx

@Nicolas-Bouteille
Copy link

  1. HTTP >> Search by name.
    I was wondering why the variable heroes$: Observable<Hero[]>; ends with a dollar sign?

@Foxandxss
Copy link
Member Author

That is explained in the text.

@Nicolas-Bouteille
Copy link

Oups! Missed that! Sorry

@Nicolas-Bouteille
Copy link

Alright I'm done with the Tutorial! Yeah!

One final note that I had regarding the last section (HTTP), compared to the previous ones, it looked more like Test Driven Development. I mean that, in the previous sections, we built components and then we included them somewhere. In the last section, we include stuff that don't exist yet, so the IDE throws error, and finally when everything is done, no more errors. Just like TDD.
Personally I like TDD, but I prefer creating components first, so that the IDE autocomplete works and does not throw errors... So in the last part I have to confess I chose to create the components and methods first before including or using them. I'm not saying that the last part should change, I'm just saying that it is different from the previous sections and that I personally felt more comfortable creating stuff before mentioning them somewhere else in my code... So just a personal feedback...

Anyway, thanks a lot for this great Tutorial guys!

@wardbell
Copy link
Contributor

wardbell commented Nov 4, 2017

@Nicolas-Bouteille

Thank you for your most generous and thorough review. I've addressed your concerns as best I can with most recent commit in the following ways.

  1. The missing instructions to add CSS in "Hero Service". Definitely needed although not where you asked for them. There are no messages at that particular point. I've added instructions down where you modify the MessagesComponent to show messages.

  2. Fixed the incorrect import instruction in "Routing > Add routes"

  3. Added the two missing "Routing" importinstructions that you mentioned.

  4. Added the backtick comment in "Routing".

  5. I struggled with where to put the "In memory web API". I have come around to your position that it belongs in the tutorial flow. I've rewritten accordingly while striving to make clear that it has nothing to do with HttpClient and you can skip this step if you're just reading the page.

  6. Added instruction for installing the angular-in-memory-web-api npm package. I'm not sure why you get those codelyzer warnings. I don't think this package is the cause.

I would like to do something about the fact that the app keeps failing while you're building it.
It's a process of breaking and fixing.

You suggested that creating the components first would alleviate the problem. I don't think so. Adding methods to the components that refer to services that don't exist yet leaves the app broken. Jumping between component and service and back again might stop some of the breakage but not all of it and I think it would be more confusing ... certainly more confusing for someone just reading the tutorial.

Therefore, having no good answer, I'm leaving it as is for now.

Thanks again!

@mary-poppins
Copy link

You can preview 1945781 at https://pr19811-1945781.ngbuilds.io/.

@Nicolas-Bouteille
Copy link

I am quite amazed to see the reactivity of the Angular community!
Congrats...

@mary-poppins
Copy link

You can preview 67e950b at https://pr19811-67e950b.ngbuilds.io/.

@Foxandxss Foxandxss added the action: merge The PR is ready for merge by the caretaker label Nov 5, 2017
@vicb vicb merged commit 049c896 into angular:master Nov 6, 2017
vicb pushed a commit that referenced this pull request Nov 6, 2017
@aaaaarrrgghhh
Copy link

I'm having the same error that Nicolas mentioned on nov 4 above, but I don't see a resolution of that.
its at this link in the tutorial https://angular.io/tutorial/toh-pt6#add-heroserviceaddhero

The addHero method results in TSLint complaining of a "Shadowed name: hero" for the declaration in "tap". I changed the variable name and its all fine now.
tap((hero1: Hero) => this.log(added hero w/ id=${hero.id})),

@jenniferfell
Copy link
Contributor

@mhevery Would you do me a favor and take a look at the comment above from a few days ago. The recommended change of variable name has not been made to the current released TOH.

Q1: Can you reproduce/verify the problem described in that comment? Or should I ask someone else (@kapunahelewong?)?
Q2: If the problem still exists, is the right procedure to reopen this PR or to create a new doc issue with the comment and proposed resolution?

Thanks.

@Foxandxss
Copy link
Member Author

I can't reproduce the problem

@Foxandxss Foxandxss deleted the aio-cli-toh branch January 4, 2018 11:08
@kapunahelewong
Copy link
Contributor

kapunahelewong commented Jan 5, 2018

This error that @aaaaarrrgghhh mentioned sounds like it's a variable that is inadvertently declared twice. Might that be the case, @aaaaarrrgghhh? (I'm guessing as I haven't had this error before.)

@jenniferfell This would be a good candidate for a new issue if @aaaaarrrgghhh can't find the cause for the error. I can't replicate the issue on my end. Everything works as expected for me.

@aaaaarrrgghhh
Copy link

Yes, I think the variable name 'hero' is defined twice in the method
/** POST: add a new hero to the server */ addHero (**hero: Hero**): Observable<Hero> { return this.http.post<Hero>(this.heroesUrl, hero, httpOptions).pipe( tap((**hero1: Hero**) => this.log(added hero w/ id=${hero.id})), catchError(this.handleError<Hero>('addHero')) ); }

@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 13, 2019
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 state: needs eng input target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants