Skip to content

docs: fix embedded view description issue#46957

Closed
AlirezaEbrahimkhani wants to merge 2 commits intoangular:mainfrom
AlirezaEbrahimkhani:fix-embedded-view-description
Closed

docs: fix embedded view description issue#46957
AlirezaEbrahimkhani wants to merge 2 commits intoangular:mainfrom
AlirezaEbrahimkhani:fix-embedded-view-description

Conversation

@AlirezaEbrahimkhani
Copy link
Contributor

@AlirezaEbrahimkhani AlirezaEbrahimkhani commented Jul 25, 2022

createEmbeddedView method instantiates an embedded view based on this template and take your templateRef as argument and would render the passed templateRef in your ViewContainerRef.

resolves #46955

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

@pullapprove pullapprove bot requested a review from alxhub July 25, 2022 07:50
@ngbot ngbot bot added this to the Backlog milestone Jul 25, 2022
jessicajaniuk
jessicajaniuk previously requested changes Jul 25, 2022
View reviewed changes
packages/core/src/linker/template_ref.ts Outdated
Copy link
Contributor

@jessicajaniuk jessicajaniuk Jul 25, 2022

Choose a reason for hiding this comment

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

This is a bit of a run on sentence that adds a bit too much. We don't really need to specify in the description here what the arguments are, as that's in the section just below this. I actually think the original text is cleaner here. Can you maybe take a second look here?

Copy link

@Achilles1515 Achilles1515 Jul 29, 2022

Choose a reason for hiding this comment

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

What about:

Instantiates an unattached embedded view based on this template.

or

Instantiates an embedded view based on this template that is unattached from a view container.

Copy link
Contributor

@jessicajaniuk jessicajaniuk Aug 5, 2022

Choose a reason for hiding this comment

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

I like the first one better. It's clear and concise.

@jessicajaniuk jessicajaniuk added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews target: patch This PR is targeted for the next patch release labels Aug 9, 2022
@AlirezaEbrahimkhani @thePunderWoman
docs: fix embedded view description issue
79c4309 
createEmbeddedView method instantiates an embedded view based on this template and take your templateRef as argument and would render the passed templateRef in your ViewContainerRef.

resolves angular#46955
@jessicajaniuk jessicajaniuk force-pushed the fix-embedded-view-description branch from 5fd6b0a to 6fc1475 Compare August 10, 2022 23:49
@jessicajaniuk jessicajaniuk requested review from AndrewKushnir and removed request for jessicajaniuk August 10, 2022 23:50
@jessicajaniuk jessicajaniuk added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Aug 10, 2022
@jessicajaniuk jessicajaniuk dismissed their stale review August 10, 2022 23:51

I've pushed a fixup making my review invalid.

@AndrewKushnir AndrewKushnir 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 labels Aug 11, 2022
@AndrewKushnir AndrewKushnir removed the request for review from alxhub August 11, 2022 00:02
@Achilles1515
Copy link

Achilles1515 commented Aug 11, 2022

@AndrewKushnir @jessicajaniuk
This should not be approved to merge.
The whole point of this PR was to remove the line about it attaching the view to a view container, because there is no view container.

@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Aug 11, 2022 •
edited
Loading

@Achilles1515 thanks for the comment. You are right, I misread the sentence, the "and attaches it to the view container" should be removed, so the description should say Instantiates an embedded view based on this template.. @jessicajaniuk could you please update the content when you get a chance?

@AndrewKushnir AndrewKushnir added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: merge The PR is ready for merge by the caretaker labels Aug 11, 2022
@Achilles1515
Copy link

Achilles1515 commented Aug 11, 2022

We were also going to add "unattached" to the description for increased clarification.

Instantiates an unattached embedded view based on this template.

@jessicajaniuk jessicajaniuk force-pushed the fix-embedded-view-description branch from 6fc1475 to 8dc2498 Compare August 11, 2022 16:21
@jessicajaniuk jessicajaniuk 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 11, 2022
@jessicajaniuk
Copy link
Contributor

jessicajaniuk commented Aug 11, 2022

@Achilles1515 Thanks for catching this before we merged it. I've replaced the fixup commit and updated with exactly that language.

@jessicajaniuk jessicajaniuk added the action: review The PR is still awaiting reviews from at least one requested reviewer label Aug 11, 2022
@AndrewKushnir AndrewKushnir 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 labels Aug 11, 2022
@pkozlowski-opensource
Copy link
Member

pkozlowski-opensource commented Aug 11, 2022

This PR was merged into the repository by commit 5509e35.

pkozlowski-opensource pushed a commit that referenced this pull request Aug 11, 2022
@AlirezaEbrahimkhani
docs: fix embedded view description issue (#46957)
063035f 
createEmbeddedView method instantiates an embedded view based on this template and take your templateRef as argument and would render the passed templateRef in your ViewContainerRef.

resolves #46955

PR Close #46957
@angular-automatic-lock-bot
Copy link

angular-automatic-lock-bot bot commented Sep 11, 2022

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 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

+2 more reviewers

@Achilles1515 Achilles1515 Achilles1515 left review comments

@jessicajaniuk jessicajaniuk jessicajaniuk left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

docs: TemplateRef.createEmbeddedView() mentions attaching the instantiated view to a view container

6 participants

@AlirezaEbrahimkhani @Achilles1515 @AndrewKushnir @jessicajaniuk @pkozlowski-opensource @thePunderWoman