-
Notifications
You must be signed in to change notification settings - Fork 870
docs(toh-5): TS/Dart review, and Dart resync #2115
Conversation
|
@wardbell @Foxandxss @brandonroberts @kwalrath: ready for review. As usual, Jade files are best viewed by ignoring whitespace. |
|
cc @kapunahelewong -- I included some of your copy edits, but mostly left my revision as is. I think that some / most of your concerns were addressed; others not (e.g. adding a complete set of new/changed files at the end of the chapter -- I was missing that too, but I can let you make the addition :). |
| import 'heroes_component.dart'; | ||
|
|
||
| // #enddocregion v2 | ||
| @Component( |
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 wondered about having two @Component annotations for a single class. Sure enough, when I try to pub serve or pub build this, I get a build error:
Build error:
Transform DirectiveProcessor on angular2_tour_of_heroes|lib/app_component_1.dart threw error: Only one Directive is allowed per class. Found unexpected "@Component(selector: 'my-app', template: '''
<h1>{{title}}</h1>
<a [routerLink]="['Heroes']">Heroes</a>
<router-outlet></router-outlet>''', directives: const [HeroesComponent], providers: const [HeroService])".
@Component(selector: 'my-app', template: '''
<h1>{{title}}</h1>
...
Does the serve/build work without errors for you?
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.
Fixing that now.
2f7822c to
fdfb011
Compare
| We'll add a third option, a `goBack` method that navigates backward one step in the browser's history stack | ||
| We'll add a third option, a `goBack` method that navigates backward one step in the browser's history stack. | ||
|
|
||
| +makeExcerpt('app/hero-detail.component.ts', 'goBack') |
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.
This is an excerpt but has no "..." to indicate that code is missing. (It's missing imports, the OnInit implementation, @Component, ...)
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 guess I'd either use the "..." or find another way to make clear that you need to import dart:html to gain access to window.
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.
Added:
We'll also need to import
dart:htmlto gain access towindow.import 'dart:html';
df68dd6 to
0ec8b82
Compare
|
LGTM! |
|
LGTM |
**NOTE: run `gulp add-example-boilerplate` after pulling in the commit.** This is preparatory work for angular#2035. As part of the the chapter review, the Dart .jade was enhanced to use Jade extends (angular#2018). By the same token it contributed to a post-RC5 resync (angular#2077). Other key changes: Dart and TS code: - Eliminated `styles.1.css` in favor of docregions in `styles.css`. - `docregion` tags renamed in a few places. - **No other code changes**. TS prose - Fixed: misnamed variable `routing` -> `appRoutes`. - All other changes are **minor copy edits**, or changes to support Dart via Jade extends. Diff of generated HTML for TS chapter was inspected to ensure only minor copy edits prevailed (i.e., that the support for Jade extends had no impact on the generated HTML).
- Some adjustments following actually doing the tutorial. In some cases code shown (e.g. this is what file foo should look like now) didn't match what the user would have. E.g., lingering @input on the hero property. - Fixed some lingering deprecated-router prose elements on TS side (e.g., still referring to a route by the old string names like `HeroDetail`). - Added extra step to `app.component.ts` creation rather than having a critical-call-out later on. - Reorder some prose for better harmony between TS and Dart prose (also improves the flow). - Moved the `styleUrls` call-out to the point of first use.
0ec8b82 to
663c5c6
Compare
663c5c6 to
9b079b7
Compare
NOTE: run
gulp add-example-boilerplateafter pulling in the commit.This PR has two major commits:
Lint reports no errors and e2e tests pass.
1. Commit: TS review and update/resync Dart
This is preparatory work for #2035. As part of the chapter review, the Dart .jade was enhanced to use Jade extends (#2108). These edits also contributed to a post-RC5 resync (#2077). Other key changes:
Dart and TS code:
styles.1.cssin favor of docregions instyles.css.docregiontags renamed in a few places.Dart code:
TS prose
HeroesComponentwas being added twice.Diff of generated HTML for TS chapter was inspected to ensure only minor copy edits prevailed (i.e., that the support for Jade extends had no impact on the generated HTML).
2. Commit: edits after doing tutorial
HeroDetail).styleUrlscall-out to the point of first use.