-
Notifications
You must be signed in to change notification settings - Fork 877
docs(toh): put a bit of clarification #1352
Conversation
`@Component` configuration object, immediately after the `template` and `styles` properties. | ||
+makeExample('toh-3/ts/app/app.component.ts', 'directives') | ||
|
||
We tell Angular about it by listing it in the metadata `directives` array. Let's add that array property to the `@Component` configuration object, |
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.
I prefer the original wording, just with the name of the file in it.
I don't see the changes to the "services" that you mention above. Where are they? |
I just changed the excerpt to say that it is an example so it shouldn't be coded in the real app. Apart from that, I exposed there my personal opinion on what we should do, but didn't do the changes. So the idea here is to agree on something so I can update this PR with those changes. |
I'm not sure I see a problem with leaving that part as it is. But if you feel strongly, let's get some other feedback too. |
The person that gave the feedback things that the gray box (that explains hierarchical injection) is a bit confusing and should be explained more "step by step". I can understand that the gray box is a bit "complicated", but that gray box is just an excerpt of another guide we have, so instead of rewording the gray box, we can point people to the original guide. There is also another part in services that talks about injections and containers and that person is confused about the word "containers". Containers is a DI concept but our DI guides never say "containers", that could be confusing. |
@Foxandxss I agree with _all_ of your points including deleting all the complicated stuff about containers and what might happen if we did x, y, or z. Just link back to the pertinent DI chapter Please make it so in this branch/PR and let me know when you want me to review it. |
@wardbell Ready. I got rid of all those excerpt that tries to explain too much about DI and hierarchical DI and replaced them with links (or nothing for the appendix). |
So @johnpapa got some feedback on the ToH and asked me to try to improve it.
Let me go step by step:
Multiple components
I put a label in the example to specify which file and also reworded the sentence a bit to tell that where to put the array is a matter of choice than a fixed place.
This part refers to the filetrees. I personally don't think that we need to explicitly say that the .js and .map files are not shown.
Services
After this bit, we have a link about "Learn more about dependency injection". Personally I don't think we can or even if we should talk more about containers in the tutorial. Truth being said, we never ever say the word "container" anymore in either "Dependency Injection" or "Hierarchical injectors" guides. Perhaps we could do some work on "Dependency Injection" to name those containers so the reader won't get confused.
This is a blue box on the guide that explains that if we want to inject
HeroService
in the child component, we shouldn't use the providers array. What I did here is to update the code example label to say that that is an example. It is easy for the reader to get confused and think that the snippet is something that they should code. So yes, no import statement because that is just an example.That being said, I think that we could get rid of the box. That is just like an excerpt of the
Dependency Injection
guide and we cannot rewrite it into smaller conceptual bites, because that is what theDependency injection
guide does. I suggest removing the blue box or making it more explicit that this is an excerpt of theDependency Injection
guide and that they can learn more there.This appendix is a continuation from the blue box and again, something that is already explained elsewhere and confuses the reader. I would drop it too.
So I updated the parts I see value in and if someone has a good feedback on the other points, I will update this PR.