Skip to content

docs(core): Add more docs of ViewChild and ViewChildren#7174

Closed
lacolaco wants to merge 1 commit into
angular:masterfrom
lacolaco:laco0416-docs-viewchild
Closed

docs(core): Add more docs of ViewChild and ViewChildren#7174
lacolaco wants to merge 1 commit into
angular:masterfrom
lacolaco:laco0416-docs-viewchild

Conversation

@lacolaco
Copy link
Copy Markdown
Contributor

Add example which is easy to understand.

cc: @alexeagle

@lacolaco lacolaco force-pushed the laco0416-docs-viewchild branch from aedd31c to b19badf Compare February 19, 2016 07:19
@lacolaco
Copy link
Copy Markdown
Contributor Author

@alexeagle Please review!

@lacolaco
Copy link
Copy Markdown
Contributor Author

@ericmartinezr Oops, you're right, thanks! It should be written following;

@ViewChildren('child1,child2,child3') children;

plunker

I'll fix it.

@lacolaco lacolaco force-pushed the laco0416-docs-viewchild branch from b19badf to e3f5313 Compare February 24, 2016 16:49
@ericmartinezr
Copy link
Copy Markdown
Contributor

@laco0416 wow, I didn't know about that feature. It's still weird though that it doesn't allow spaces between the commas. I think you should specify for that case that there must be no spaces, or send a PR fixing it and allowing zero, one or more spaces.

In either case, thanks for the hint! Didn't know about it!

@lacolaco lacolaco force-pushed the laco0416-docs-viewchild branch from e3f5313 to e52007d Compare February 24, 2016 16:51
@lacolaco
Copy link
Copy Markdown
Contributor Author

@ericmartinezr Yes, I think also as you say. I'll send another PR for it ASAP!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This sentence doesn't make any sense to me. The old text is better IMHO. Even when the annotation name was changed from ViewQuery to ViewChildren it is still a query. Using the name of the element to document as main part of the documentation doesn't provide much value anyway. ("Foo" "This is a Foo")

What about something like

"When a field is decorated with @ViewChildren() Angular assigns a QueryList containing references to the elements that match the name or type of the argument passed to @ViewChildren(). Changes in the DOM that affect resulting elements update the content of the QueryList." (I hope the last sentence is actually true, but I think so)

Copy link
Copy Markdown
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. I'd like to use your sentence.

and, the last sentence is actually true. look at this! plunker

@alexeagle
Copy link
Copy Markdown
Contributor

I'm not the subject matter expert here to review the correctness of what you've added, so attempting to dispatch...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same, I can't imagine anyone getting any idea what this is about from this description.

"When a field is decorated with @ViewChild() Angular assigns a component, directive or ElementRef to the field. When the parameter passed to @ViewChild() is a string, an element with a template variable with this name is looked up in the view and a reference to that element is assigned to the field. If a type is passed as argument a directive or component with that type is looked up in the view and if found a reference to the directive or component instance is assigned to the field."

Not sure this is 100% correct though. The information about the parameters also applies to @ViewChildren and should probably be added there too.

@lacolaco
Copy link
Copy Markdown
Contributor Author

@alexeagle oh, sorry. because I found your TODO comments in the code, I thought you are the best reviewer.

@lacolaco lacolaco force-pushed the laco0416-docs-viewchild branch from 2bdc73e to 8993129 Compare February 25, 2016 14:37
@lacolaco
Copy link
Copy Markdown
Contributor Author

@tbosch Please review!
cc: @zoechi

@lacolaco lacolaco force-pushed the laco0416-docs-viewchild branch from 8993129 to e9588f6 Compare February 25, 2016 14:48
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm wondering what happens when more elements match. Does it just assign the first it finds?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes, thanks! I'll add a sentence to explain it.

@zoechi
Copy link
Copy Markdown
Contributor

zoechi commented Feb 25, 2016

That's what I call documentation :)
The grammar might need some improvement but I'm the wrong guy for that task.
I think the content is correct and explains it well, especially with the code examples.
(I added one comment above about @ViewChild matching more than one element)
otherwise LGTM

@lacolaco lacolaco force-pushed the laco0416-docs-viewchild branch from 8f21d97 to 04d17a5 Compare February 26, 2016 01:28
@lacolaco
Copy link
Copy Markdown
Contributor Author

@zoechi thank you for your kind comments! 😃

@lacolaco
Copy link
Copy Markdown
Contributor Author

@tbosch Please review :)

Comment thread modules/angular2/src/core/metadata.ts Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@alexeagle why did we duplicate this? So that tools show the docs correctly when writing an example?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yes. in fact we have not duplicated enough - the properties of the object literal argument to our decorators don't have docs appearing in the editor

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

so, should I keep duplicated docs?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can you leave the TODO for me to fix this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Okey!

@tbosch tbosch added action: review The PR is still awaiting reviews from at least one requested reviewer action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Mar 11, 2016
@lacolaco lacolaco force-pushed the laco0416-docs-viewchild branch from acf41b3 to c61d688 Compare March 11, 2016 15:02
@lacolaco
Copy link
Copy Markdown
Contributor Author

@tbosch I fixed docs!

@lacolaco
Copy link
Copy Markdown
Contributor Author

@tbosch ping

@lacolaco
Copy link
Copy Markdown
Contributor Author

@tbosch can you review again?

cc/ @mhevery

@alexeagle
Copy link
Copy Markdown
Contributor

Please squash the commits and fix the travis, sorry this is taking so long, we are really busy and Tobias is the expert on this

@alexeagle alexeagle added pr_state: LGTM and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Apr 12, 2016
@lacolaco lacolaco force-pushed the laco0416-docs-viewchild branch from c61d688 to aa35ab5 Compare April 12, 2016 23:52
@lacolaco lacolaco force-pushed the laco0416-docs-viewchild branch from aa35ab5 to bc54ac6 Compare April 13, 2016 00:02
@lacolaco
Copy link
Copy Markdown
Contributor Author

@tbosch done!

Thank you @alexeagle

@alexeagle alexeagle added action: merge The PR is ready for merge by the caretaker 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 Apr 13, 2016
@lacolaco
Copy link
Copy Markdown
Contributor Author

thanks mate!

@lacolaco lacolaco deleted the laco0416-docs-viewchild branch April 13, 2016 23:11
@angular-automatic-lock-bot
Copy link
Copy Markdown

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 8, 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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants