Skip to content

docs: rewrite introduction to dependency injection#44466

Closed
ileil wants to merge 1 commit intoangular:mainfrom
ileil:di_intro
Closed

docs: rewrite introduction to dependency injection#44466
ileil wants to merge 1 commit intoangular:mainfrom
ileil:di_intro

Conversation

@ileil
Copy link
Contributor

@ileil ileil commented Dec 13, 2021

Rewriting dependency injection introduction to comply with new documentation templates.

learn2grid
learn2grid previously approved these changes Mar 12, 2022
@ileil ileil force-pushed the di_intro branch 2 times, most recently from 303a826 to 4f653ff Compare March 21, 2022 17:30
@mary-poppins
Copy link

You can preview 4f653ff at https://pr44466-4f653ff.ngbuilds.io/.

@ileil ileil marked this pull request as ready for review March 24, 2022 18:02
@pullapprove pullapprove bot requested review from andrewseguin, atscott, gkalpak and mgechev and removed request for aikithoughts March 24, 2022 18:02
@mary-poppins
Copy link

You can preview bc5b9ba at https://pr44466-bc5b9ba.ngbuilds.io/.

Copy link
Contributor

@TMDavisGoogle TMDavisGoogle Mar 24, 2022

Choose a reason for hiding this comment

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

Line 61: There is a specific way to enter an em dash in Cider.

@mary-poppins
Copy link

You can preview 76446c8 at https://pr44466-76446c8.ngbuilds.io/.

@mary-poppins
Copy link

You can preview f4425bb at https://pr44466-f4425bb.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 746483f at https://pr44466-746483f.ngbuilds.io/.

@mary-poppins
Copy link

You can preview f1c9e1b at https://pr44466-f1c9e1b.ngbuilds.io/.

@ileil ileil force-pushed the di_intro branch 3 times, most recently from 3da66f1 to 82cf550 Compare July 18, 2022 15:50
@ileil ileil requested a review from AndrewKushnir July 27, 2022 15:59
@mary-poppins
Copy link

You can preview cfa0a57 at https://pr44466-cfa0a57.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 3484489 at https://pr44466-3484489.ngbuilds.io/.

@mary-poppins
Copy link

You can preview d1ee786 at https://pr44466-d1ee786.ngbuilds.io/.

Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

@ileil thanks for updating the content 👍 I've left a few comments, please let me know if any additional info is needed.

I also noticed that the CI is failing and the failures are related to the changes in this PR. Please let me know if you need help debugging them.

Comment on lines 30 to 53
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is still relevant: these references point nowhere, since the corresponding <a> elements were removed. I think we can just drop these from here as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

This content duplicates the title of this section. I think it might be better to say "Learn more" here (and in other cases below):

Suggested change
<p class="card-footer">Understanding Dependency Injection</p>
<p class="card-footer">Learn more</p>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the style writers are following. I'll keep them as is for consistency for now. Will bring this up in the next doc meeting.

Copy link
Contributor

@AndrewKushnir AndrewKushnir Aug 3, 2022

Choose a reason for hiding this comment

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

Ok, thanks. Btw, it's different on the main page (which makes sense to me): https://angular.io/docs

Copy link
Contributor

Choose a reason for hiding this comment

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

Note: I think the file should be renamed from "inject-object.md" to something else, since the content talks about other types of content aside from objects.

Copy link
Contributor

Choose a reason for hiding this comment

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

Btw, I don't think we had that page at all in our Google Doc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. This is an obsolete file which I kept in the repo but removed from the navigation, in case we need to reuse some content from this file. I just removed it from the repo.

@mary-poppins
Copy link

You can preview 2a96eab at https://pr44466-2a96eab.ngbuilds.io/.

Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

@ileil thanks for additional updates. The content looks good from technical perspective, for I LGTM it. It'd be great if someone can review from the docs perspective as well.

Before landing the PR we should also make sure the CI is "green" and all commits are merged into one (or a few commits).

Thank you.

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI the backticks are displayed as is in the description: https://pr44466-2a96eab.ngbuilds.io/guide/dependency-injection-overview. We should probably remove them from here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed backticks from the doc cards.

@mary-poppins
Copy link

You can preview 9b3f6a5 at https://pr44466-9b3f6a5.ngbuilds.io/.

@mary-poppins
Copy link

You can preview fd0b3ff at https://pr44466-fd0b3ff.ngbuilds.io/.

@mary-poppins
Copy link

You can preview ce62b59 at https://pr44466-ce62b59.ngbuilds.io/.

@bob-watson bob-watson removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Aug 4, 2022
@mary-poppins
Copy link

You can preview 4aa2689 at https://pr44466-4aa2689.ngbuilds.io/.

@mgechev mgechev added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer action: merge The PR is ready for merge by the caretaker labels Aug 4, 2022
Refactored the DI section to improve doc quality, reduce cognitive load and drive consistency.

- Added an overview with prerequisites and doc cards that point to rest of the DI content
- Added introduction topic with topic purpose, value proposition and "fail fast"
- Broke apart complex concepts into simpler tasks
- Unified tone and language for each topic
- Added new content based on SME feedback
- Deleted obsolete content
@mary-poppins
Copy link

You can preview 298f592 at https://pr44466-298f592.ngbuilds.io/.

@atscott
Copy link
Contributor

atscott commented Aug 5, 2022

This PR was merged into the repository by commit 0920a15.

@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.

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 action: review The PR is still awaiting reviews from at least one requested reviewer core: di merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note 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