docs(architecture): proofread, updated Dart app #1807

chalin · 2016-07-01T15:47:31Z

Note that there are two commits: one for each of TS- and Dart-side changes.

e2e tests now also cover the tax calculator.
Dart app updated to match TS (it had no sales tax calculator).
TS sample source cleanup (e.g. removed many unnecessary docregions).
Prose updated to include @kwalrath's revisions from a while ago, with some of my edits as well.

Contributes to #1598 and #1508.

chalin · 2016-07-01T15:50:20Z

@kwalrath @thso : Dart-side commit ready for review.
@Foxandxss @wardbell : TS-side commit ready for review. As usual, it is best to review the .jade file while ignoring changes in indentation. Note that most of the TS Jade file edits were from Kathy's work from a while back.

kwalrath · 2016-07-01T15:58:01Z

public/docs/dart/latest/guide/architecture.jade

+    header Dart difference: Module is a compilation unit
+    :marked
+      The term "module" used in this guide refers to a
+      Dart _compilation unit_, i.e., a _library_ or _part_.


i.e. -> that is

(many people don't know what i.e. means)

Actually, though... do we really need to mention parts? They're strongly discouraged. Also, people might not know that each file without a library directive is itself a library. Perhaps instead:

In this guide, the term module refers to a Dart compilation unit, such as a library. If a Dart file has no library or part directive, then that file itself is a library and thus a compilation unit. For more information...

Fixed as you suggested with tweaks we agree to elsewhere (in particular clarifying that a Dart package is also a module).

kwalrath · 2016-07-01T17:52:11Z

Looking good! Once you address a few comments, the Dart side is good to go.

chalin · 2016-07-01T19:14:46Z

Dart-side issues addressed.

kwalrath · 2016-07-01T21:23:45Z

LGTM! @Foxandxss or @wardbell, who's going to review the TS side?

Foxandxss · 2016-07-04T15:25:07Z

public/docs/_examples/architecture/dart/lib/sales_tax_component.dart

+    selector: 'sales-tax',
+    template: '''
+      <h2>Sales Tax Calculator</h2>
+      Amount: <input #amountBox (input)="0" type="number">


What triggered this change (also in TS)? I am not sure I like (input) as the event name. I don't know, I guess it is just me but I liked (change) more. Input is also the element name so as an event again is weird. Also input sounds more like an input (pun intended) than as an output.

On the other hand, the number input is really broken in other browsers. I don't think they are fixed yet.

Triggering on onInput rather than onChange makes for a more responsive user experience (UX). Again, using type="number" makes for a better UX in those browsers that support it, and makes no difference (from the app processing point of view) in those browsers that do not support it.

Please revert to (change). We're not trying to be responsive. We're trying to make a point about how (someEvent)="0" is a trick to cause dirty checking when the user enters something. (input) invites the kind of distracting question that Jesus asked.

Please revert type="number". This is a distraction from the point. Again, we're not looking to produce the best app solution here. We're trying to make a narrow point. Let's not add distractions from that point.

Ok. Undone.

wardbell · 2016-07-06T04:47:57Z

public/docs/ts/latest/guide/architecture.jade

 :marked
  Perhaps the first module we meet is a module that exports a *component* class.
  The component is one of the basic Angular blocks, we write a lot of them,
  and we'll talk about components in the next segment. For the moment it is enough to know that a
  component class is the kind of thing we'd export from a module.

-  Most applications have an `AppComponent`. By convention, we'll find it in a file named `app.component.ts`.
+  Most applications have an `AppComponent`. By convention, we'll find it in a file named `!{_app_comp_filename}`.
  Look inside such a file and we'll see a declaration like this one.


Replace like with such as:
Look inside such a file and we'll see a declaration such as this one.

wardbell · 2016-07-06T04:57:04Z

Please update ts/architecture.jade as follows:
line 84: "Look inside such a file and we'll see a declaration such as this one."
line 384: "They tend to appear within an element tag as attributes do,"

wardbell · 2016-07-06T05:03:54Z

I am uncomfortable with certain changes in the e2e tests, most especially the tests-that-are-not-tests such as

  it(`selects ${targetHero.name} from hero list`, function () {
    let hero = element(by.cssContainingText('li', targetHero.name));
    hero.click();
    // Nothing specific to expect other than lack of exceptions.
  });

and

  it(`can update hero name`, () => {
    addToHeroName(nameSuffix);
    // Nothing specific to expect other than lack of exceptions.
  });

The second is also not accepted because it delegates to addToHeroName, a helper function that is used exactly ONCE, in this place only. That move detracts from readability and maintenance w/o compensating benefits.

I think I know why you created these tests; they are a clever way to slip in an async operation w/o waiting for it. But they depend upon test order which test suites should not do. Move one of these tests and the suite fails.

I realize that a great number of our e2e tests have this failing (when you see beforeAll loading the browser just once, you can guess that cross-test pollution is a likely risk). I accept that to make these tests faster. But I don't accept splitting what is actually ONE test over two tests in order to avoid the async setup operation. Please think of a better way.

wardbell · 2016-07-06T05:06:32Z

Unfortunately, this PR is NOT based on an IdeaBlade branch. That means I can't fix it.

You and Kathy have the ability to add branches to ideablade/angular.io.

Please base all future PRs on branches in ideablade/angular.io or at least all future branches that involve this many changes. That has been the long standing practice for members of the author team.

wardbell · 2016-07-06T05:15:02Z

public/docs/_examples/architecture/ts/app/hero-list.component.html

-<div *ngFor="let hero of heroes" (click)="selectHero(hero)">
-  {{hero.name}}
-</div>
+<ul>


Why switch from <div> to <ul><li>? That a distraction. I wanted less code on the page. I didn't want to have to deal with the bullets of a <ul>.

I'm not going to insist on reverting this. But I think you are mistakenly trying to improve the code at the expense of the clarity of the Template Syntax.

"LESS HTML IS MORE"

chalin · 2016-07-06T14:57:45Z

@wardbell : thanks for the feedback. I believe that all of your concerns have been addressed in the last commit (f38496f). Let me know if there is anything else.

@kwalrath

- e2e tests now also cover the tax calculator. - Dart app updated to match TS (it had no sales tax calculator). - TS sample source cleanup (e.g. removed many unnecessary `docregions`). - Prose updated to include @kwalrath's revisions from a while ago, with some of my edits as well. Contributes to angular#1598 and angular#1508.

@kwalrath

- e2e tests now also cover the tax calculator. - Dart app updated to match TS (it had no sales tax calculator). - TS sample source cleanup (e.g. removed many unnecessary `docregions`). - Prose updated to include @kwalrath's revisions from a while ago, with some of my edits as well. Contributes to angular#1598 and angular#1508.

wardbell · 2016-07-12T03:33:57Z

Thanks. Merging.

googlebot added the CLA: yes label Jul 1, 2016

kwalrath reviewed Jul 1, 2016
View reviewed changes

chalin force-pushed the chalin-architecture-review-0630 branch from 5488fec to f20221c Compare July 1, 2016 19:00

This was referenced Jul 1, 2016

Update Dart Basics pages to link to live example and source code #1598

Closed

Update Basics to new design scheme using Jade extends/blocks #1508

Closed

chalin force-pushed the chalin-architecture-review-0630 branch 3 times, most recently from 70f6eeb to a59aafe Compare July 3, 2016 21:03

Foxandxss reviewed Jul 4, 2016
View reviewed changes

wardbell reviewed Jul 6, 2016
View reviewed changes

wardbell removed the pr_state: LGTM label Jul 6, 2016

wardbell reviewed Jul 6, 2016
View reviewed changes

chalin force-pushed the chalin-architecture-review-0630 branch 2 times, most recently from 1c87901 to f38496f Compare July 6, 2016 14:55

kwalrath added the pr_state: LGTM-dart label Jul 7, 2016

chalin added 4 commits July 7, 2016 21:58

docs(architecture/dart): post-review changes

7cc2b5d

docs(architecture/ts): post-review updates

5b53d6b

chalin force-pushed the chalin-architecture-review-0630 branch from f38496f to 5b53d6b Compare July 8, 2016 04:58

wardbell closed this in 761f857 Jul 12, 2016

chalin deleted the chalin-architecture-review-0630 branch July 12, 2016 15:56

docs(architecture): proofread, updated Dart app #1807

docs(architecture): proofread, updated Dart app #1807

Uh oh!

Conversation

chalin commented Jul 1, 2016

Uh oh!

chalin commented Jul 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kwalrath commented Jul 1, 2016

Uh oh!

chalin commented Jul 1, 2016

Uh oh!

kwalrath commented Jul 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wardbell commented Jul 6, 2016

Uh oh!

wardbell commented Jul 6, 2016

Uh oh!

wardbell commented Jul 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chalin commented Jul 6, 2016

Uh oh!

wardbell commented Jul 12, 2016

Uh oh!

Uh oh!

wardbell commented Jul 6, 2016 •

edited

Loading