Skip to content
This repository was archived by the owner on Dec 4, 2017. It is now read-only.

docs(toh-5): TS/Dart review, and Dart resync #2115

Merged
merged 5 commits into from
Aug 17, 2016

Conversation

chalin
Copy link
Contributor

@chalin chalin commented Aug 14, 2016

NOTE: run gulp add-example-boilerplate after pulling in the commit.

This PR has two major commits:

  1. Post-RC5 resync
  2. Edits after having done the tutorial.

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:

  • Eliminated styles.1.css in favor of docregions in styles.css.
  • docregion tags renamed in a few places.
  • No other code changes.

Dart code:

  • Include renamed constructor name in "renamings" figure.

TS prose

  • Fix: HeroesComponent was being added twice.
  • 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).

2. Commit: edits after doing tutorial

  • 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.
  • Added extra step to app.component.ts creation rather than having a critical-call-out later on.
  • TS-side: fixed some lingering deprecated-router prose elements (e.g., still referring to a route by the old string names like HeroDetail).
  • 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.

@chalin
Copy link
Contributor Author

chalin commented Aug 14, 2016

@wardbell @Foxandxss @brandonroberts @kwalrath: ready for review.

As usual, Jade files are best viewed by ignoring whitespace.

@chalin
Copy link
Contributor Author

chalin commented Aug 14, 2016

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 'hero_service.dart';
import 'heroes_component.dart';

// #enddocregion v2
@Component(
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixing that now.

@chalin chalin force-pushed the chalin-dart-toh-5-0812 branch from 2f7822c to fdfb011 Compare August 15, 2016 21:00
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')
Copy link
Contributor

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, ...)

Copy link
Contributor

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.

Copy link
Contributor Author

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:html to gain access to window.

import 'dart:html';

@chalin chalin force-pushed the chalin-dart-toh-5-0812 branch 2 times, most recently from df68dd6 to 0ec8b82 Compare August 15, 2016 22:51
@kwalrath
Copy link
Contributor

LGTM!

@Foxandxss
Copy link
Member

LGTM

chalin added 2 commits August 17, 2016 12:19
**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.
@chalin chalin force-pushed the chalin-dart-toh-5-0812 branch from 0ec8b82 to 663c5c6 Compare August 17, 2016 19:22
@chalin chalin force-pushed the chalin-dart-toh-5-0812 branch from 663c5c6 to 9b079b7 Compare August 17, 2016 20:07
@kwalrath kwalrath merged commit 0c0c6f6 into angular:master Aug 17, 2016
@kwalrath kwalrath deleted the chalin-dart-toh-5-0812 branch August 17, 2016 20:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants