Skip to content

Conversation

@dogeared
Copy link

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
[x] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

Issue Number: #19714

What is the new behavior?

Adds a little descriptive text in the Expose heroes section of part 2 of the tutorial

Does this PR introduce a breaking change?

[ ] Yes
[x] No


### Expose heroes
Create a public property in `AppComponent` that exposes the heroes for binding.
Create a public property in `AppComponent` (between the existing `title` and `hero` properties) that exposes the heroes for binding.
Copy link
Member

Choose a reason for hiding this comment

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

As discussed in #19714, the order of properties doesn't make any difference. This change makes it sound as if there are more constraints than there actually are. IMO "create a public property in AppComponent" is adequately describing what needs to be done.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kapunahelewong and I were just discusssing. What about something like this: Create a public property in 'AppComponent' that exposes the heroes for binding. To make it easier to read your code, keep all public properties together. For this tutorial, define [create? put?] the 'heroes' property between the existing 'title' and 'hero' properties.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dogeared We could also expand the docregion for the example code to show it in context.

Copy link
Author

Choose a reason for hiding this comment

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

I updated the PR per your suggestions.

@gkalpak gkalpak added comp: docs action: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 16, 2017
@dogeared
Copy link
Author

dogeared commented Oct 16, 2017 via email

@LinboLen
Copy link

can words be a little bit softer?

Copy link
Contributor

@kapunahelewong kapunahelewong left a comment

Choose a reason for hiding this comment

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

LGTM

@kapunahelewong
Copy link
Contributor

@LinboLen Would you like to make a suggestion for @dogeared to consider?

@gkalpak
Copy link
Member

gkalpak commented Oct 16, 2017

In other places, the tutorial is explicit about where to place things as it makes it more clear. For instance: https://angular.io/tutorial/toh-pt1#hero-object
It's not technically critical that the new Hero class goes right below the import statement.

It is actually critical. Since that file only contains import statements and the AppComponent class, and since classes are not hoisted (like function), it is important to put the Hero class before the AppComponent class (and below the import statements, which according to the spec should be at the top).

The new wording is fine by me. Just for the recond, imo making it sound like there are more limitations than necessary tends to confuse newcomers down the road.

👍 for expanding the docregions. In fact, I think it might be useful to introduce a new feature for context in docregions that show adding to or modifying an existing file. I.e. 1-2 lines above and below the affected lines would appear grayed-out (to show what was there before the change), while the affected lines would be highlighted in color as usual.

@ngbot
Copy link

ngbot bot commented Jan 16, 2018

Hello? Don't want to hassle you. Sure you're busy. But this PR has some merge conflicts that you probably ought to resolve.
That is... if you want it to be merged someday...

1 similar comment
@ngbot
Copy link

ngbot bot commented Jan 16, 2018

Hello? Don't want to hassle you. Sure you're busy. But this PR has some merge conflicts that you probably ought to resolve.
That is... if you want it to be merged someday...

@jenniferfell
Copy link
Contributor

@dogeared : Hi. First, let me apologize for this sitting so long. I agree that this change is very helpful to readers. Would you be willing to resolve the merge conflicts? We'd be super appreciative, and if you tag me when you're done, I'll make sure to get it merged. :-)

@jenniferfell jenniferfell added state: community Someone from the Angular community is working on this issue or submitted this PR 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: review The PR is still awaiting reviews from at least one requested reviewer labels May 25, 2018
@jenniferfell
Copy link
Contributor

@dogeared Hi. I investigated this some more. It's still a great idea, and I like your wording. Things have moved around a bit since you submitted this PR. We're happy to incorporate your change into another PR we're working on (#24445). Or...if you'd prefer to update this PR, we'll help you move it forward this time. Thanks again for this submission, and making us aware of a general theme we should be better about: where to do things. Thanks again.

@jenniferfell
Copy link
Contributor

Also, I opened a new issue for the larger question of how can we show more context in code regions. Thx also to @gkalpak

@jenniferfell jenniferfell added state: community Someone from the Angular community is working on this issue or submitted this PR and removed state: community Someone from the Angular community is working on this issue or submitted this PR labels Jul 23, 2018
@brandonroberts
Copy link
Contributor

Closing as no longer applicable

@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: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews cla: yes state: community Someone from the Angular community is working on this issue or submitted this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants