Fix aio minor fixes #21695

gkalpak · 2018-01-22T11:10:34Z

This PR fixes several issues. See the commits/commit messages for more details.
(Should be easier to review commit-by-commit and ignoring whitespace in some of them.)

In a nutshell:

Removes links from sub-menu headings (toggles).
Reduces flicker/reflows/animations for initial rendering.
Fixes header-links on narrow screens.
Ignores header-links when selecting the heading text.

WRT header-links on narrow screens...

Before:

After:

mary-poppins · 2018-01-22T11:17:45Z

You can preview 17bbc2f at https://pr21695-17bbc2f.ngbuilds.io/.

petebacondarwin · 2018-01-31T14:38:57Z

aio/content/navigation.json

@@ -74,7 +74,6 @@
    },

    {
-      "url": "tutorial",


petebacondarwin · 2018-01-31T14:44:36Z

aio/src/app/app.component.spec.ts

+    await newDocPromise;       // Wait for the new document to be fetched.
+    fixture.detectChanges();   // Propagate document change to the view (i.e to `DocViewer`).
+    await docRenderedPromise;  // Wait for the `docRendered` event.
+  };


Nice helper!

But why use

const awaitDocRendered = async () => { ... };

rather than

async function awaitDocRendered() { ... }

Because function is noso ES5 😛

(The real answer is that I was keeping consistent with what we had before: const initializeTest = () => {. Changed to function 😁)

(Updated the previous comment 😒)

petebacondarwin · 2018-01-31T14:47:58Z

aio/src/app/app.component.spec.ts

+    await docRenderedPromise;  // Wait for the `docRendered` event.
+  };
+
+  const initializeTest = (waitForDoc = true) => {


similarly here, why not: function initializeText(waitForDoc = true) { ... }?

petebacondarwin · 2018-01-31T14:58:57Z

aio/src/app/app.component.spec.ts

-
-        initializeTest();
+        initializeTest(false);
+        jasmine.clock().tick(1);  // triggers the HTTP response for the document


Are you using jasmine clocks here because the animations can't be tested using fakeAsync?

I don't recall tbh. I think the main reason was that fakeAsync() barks on pending timeouts (and we have a bunch on them in AppComponent), so you have to flush unrelated timeouts all the time.

I generally find jasmine clocks to work fine for these simple things (e.g. not interested in microtasks, etc). But I am pretty sure fakeAsync() would work as well (with some extra flushing/ticking at the end of each test).

Another issue with fakeAsync is that you shouldn't have async stuff (e.g. timers) going on outside of fakeAsync, so it is difficult to share beforeEach blocks with tests that don't use fakeAsync() (but I don't think this affects us in this particular case).

petebacondarwin · 2018-01-31T14:59:34Z

aio/src/app/app.component.spec.ts

-    return of(data);
+
+    // Preserve async nature of `HttpClient`.
+    return timer(1).mapTo(data);


petebacondarwin · 2018-01-31T15:03:46Z

aio/src/app/app.component.ts

@@ -57,9 +57,11 @@ export class AppComponent implements OnInit {
  @HostBinding('class')
  hostClasses = '';

-  isFetching = false;
+  // Disable all Angular animations for the initial render.
+  @HostBinding('@.disabled')


petebacondarwin · 2018-01-31T15:05:14Z

aio/src/app/app.component.ts

+    if (this.isStarting) {
+      // In order to ensure that the initial sidenav-content left margin
+      // adjustment happens without animation, we need to ensure that
+      // `isStarting` remain `true` until the margin change is triggered.


remain -> remains

petebacondarwin · 2018-01-31T16:35:02Z

aio/src/styles/1-layouts/_sidenav.scss

-.starting.mat-sidenav-transition {
-  .mat-sidenav,
-  .mat-sidenav-content,
-  .mat-sidenav-backdrop.mat-sidenav-shown {


So has the behaviour of Angular Material changed, so that only the drawer is animated now? Or were we wrong previously about this, and were capturing all these elements to block their animation to make up for it?

Yes, the Material styles have changed so that the transition (which we want to overwrite here) is on .mat-drawer-content: angular/components@372436c#diff-20a207a17a517c2beae586b174bf14b7L4
(We could probably keep the previous classes .mat-sidenav-transition/.mat-sidenav-content, since they are present on the element too, but it felt safer to match the Material selectors.)

The .mat-sidenav animation has been switched to @angular/animations for a while now, so these CSS styles didn't have any effect.

Finally, the backdrop is only visible on narrow screen where we don't have the sidenav and content side-by-side, but the sidenav is always closed on these narrow screens (even when landing on sidenav page), so the backdrop was never shown anyway.

petebacondarwin · 2018-01-31T16:41:13Z

aio/src/styles/2-modules/_heading-anchors.scss

-    .mat-icon, .material-icons {
-      visibility: hidden;
-      display: inline-block;
+    .header-link {


The commit that contains this change appears to have the wrong commit message.

petebacondarwin

Just a typo, a commit message that needs changing and a few questions.

mary-poppins · 2018-02-01T12:40:20Z

You can preview 4d1ec60 at https://pr21695-4d1ec60.ngbuilds.io/.

gkalpak · 2018-02-01T12:42:55Z

Thx for the review, @petebacondarwin.
Addressed comments. Will mark for merge as soon as Travis is green.

gkalpak · 2018-02-01T12:52:17Z

Hm...something does not look right when loading the home page (there is white margin on the left briefly).
Looking into it...

mary-poppins · 2018-02-01T17:55:07Z

You can preview 4b61c69 at https://pr21695-4b61c69.ngbuilds.io/.

gkalpak · 2018-02-01T18:56:30Z

Looks much better now. One test fails on Travis (although it passes locally 😒).

petebacondarwin · 2018-02-01T20:09:24Z

At least it is a clear error message.

 ✗ should not navigate when clicking on nav-item headings (sub-menu toggles)
      - Failed: unknown error: Element <button class="vertical-menu-item heading level-1 collapsed ng-star-inserted" type="button" ng-reflect-klass="vertical-menu-item heading" ng-reflect-ng-class="[object Object]" title="Techniques for putting Angular to work in your environment" aria-pressed="false">...</button> is not clickable at point (135, 377). Other element would receive the click: <button class="vertical-menu-item heading ng-star-inserted level-2 collapsed" type="button" ng-reflect-klass="vertical-menu-item heading" ng-reflect-ng-class="[object Object]" title="Dependency Injection: creating and injecting services" aria-pressed="false">...</button>

petebacondarwin · 2018-02-01T20:12:05Z

Testing locally here...

petebacondarwin · 2018-02-01T20:16:39Z

Works locally for me too :-/

gkalpak · 2018-02-01T20:23:19Z

It is an random error. It happened 3/4 times on Travis. Related: angular/protractor#3093

mary-poppins · 2018-02-01T21:08:06Z

You can preview 338e144 at https://pr21695-338e144.ngbuilds.io/.

@Maxkorz

Implemented @Maxkorz's [suggestion](#21515 (comment)). Fixes #21515 PR Close #21695

Navigating to a document while trying to expand or collapse a sub-menu is undesirable and confusing. All sub-menu toggles should have no other effect than expanding/collapsing the corresponding sub-menu. PR Close #21695

Previously, the mocked `HttpClient` was synchronous in tests (despite the actual `HttpClient` being asynchronous). Although we use observables (which generally make the implementation sync/async-agnostic), the fact that we have no control over when Angular updates/checks views and calls lifecycle hooks resulted in different behavior (and errors) in tests (with sync `HttpClient`) vs actual app (with async `HttpClient`). This commit ensures that the behavior (and errors) are consistent between the tests and the actual app by making the mocked `HttpClient` asynchronous. PR Close #21695

For the initial rendering, where there is no transition from a previous visual state to a new one, animations make little sense. The page should load with as few reflows as possible. Similarly, while we typically want to defer updating the SideNav state (e.g. opened/closed) until the "leaving" document is animated out of the page, on the initial rendering (where there is no "leaving" document) this leads to the SideNav flashing (from closed to open). These worked as expected before, but several parts (mostly related to documents with a SideNav) have been accidentally broken in recent commits (e.g. when upgraded to latest material, or enabled animations for DocViewer transitions, etc.). This commit restores the previous behavior by ensuring that (on the initial rendering) the SideNav state is updated as soon as possible and that there will be no animations when: 1. The hamburger button appears. 2. The SideNav is opened. 3. The main section's width is adjusted to make room for the SideNav. PR Close #21695

PR Close #21695

During the initial load of the page (probably until the icon styles are loaded and/or applied), the `.header-link` element is wider, pushing the heading text slightly to the right (for a brief moment). This commit prevents this slight shift by explicitly setting the width for the `.header-link` element. PR Close #21695

@Maxkorz

Implemented @Maxkorz's [suggestion](#21515 (comment)). Fixes #21515 PR Close #21695

PR Close angular#21695

Navigating to a document while trying to expand or collapse a sub-menu is undesirable and confusing. All sub-menu toggles should have no other effect than expanding/collapsing the corresponding sub-menu. PR Close angular#21695

…#21695) Previously, the mocked `HttpClient` was synchronous in tests (despite the actual `HttpClient` being asynchronous). Although we use observables (which generally make the implementation sync/async-agnostic), the fact that we have no control over when Angular updates/checks views and calls lifecycle hooks resulted in different behavior (and errors) in tests (with sync `HttpClient`) vs actual app (with async `HttpClient`). This commit ensures that the behavior (and errors) are consistent between the tests and the actual app by making the mocked `HttpClient` asynchronous. PR Close angular#21695

…1695) For the initial rendering, where there is no transition from a previous visual state to a new one, animations make little sense. The page should load with as few reflows as possible. Similarly, while we typically want to defer updating the SideNav state (e.g. opened/closed) until the "leaving" document is animated out of the page, on the initial rendering (where there is no "leaving" document) this leads to the SideNav flashing (from closed to open). These worked as expected before, but several parts (mostly related to documents with a SideNav) have been accidentally broken in recent commits (e.g. when upgraded to latest material, or enabled animations for DocViewer transitions, etc.). This commit restores the previous behavior by ensuring that (on the initial rendering) the SideNav state is updated as soon as possible and that there will be no animations when: 1. The hamburger button appears. 2. The SideNav is opened. 3. The main section's width is adjusted to make room for the SideNav. PR Close angular#21695

PR Close angular#21695

During the initial load of the page (probably until the icon styles are loaded and/or applied), the `.header-link` element is wider, pushing the heading text slightly to the right (for a brief moment). This commit prevents this slight shift by explicitly setting the width for the `.header-link` element. PR Close angular#21695

@Maxkorz

…lar#21695) Implemented @Maxkorz's [suggestion](angular#21515 (comment)). Fixes angular#21515 PR Close angular#21695

PR Close angular#21695

Navigating to a document while trying to expand or collapse a sub-menu is undesirable and confusing. All sub-menu toggles should have no other effect than expanding/collapsing the corresponding sub-menu. PR Close angular#21695

…#21695) Previously, the mocked `HttpClient` was synchronous in tests (despite the actual `HttpClient` being asynchronous). Although we use observables (which generally make the implementation sync/async-agnostic), the fact that we have no control over when Angular updates/checks views and calls lifecycle hooks resulted in different behavior (and errors) in tests (with sync `HttpClient`) vs actual app (with async `HttpClient`). This commit ensures that the behavior (and errors) are consistent between the tests and the actual app by making the mocked `HttpClient` asynchronous. PR Close angular#21695

…1695) For the initial rendering, where there is no transition from a previous visual state to a new one, animations make little sense. The page should load with as few reflows as possible. Similarly, while we typically want to defer updating the SideNav state (e.g. opened/closed) until the "leaving" document is animated out of the page, on the initial rendering (where there is no "leaving" document) this leads to the SideNav flashing (from closed to open). These worked as expected before, but several parts (mostly related to documents with a SideNav) have been accidentally broken in recent commits (e.g. when upgraded to latest material, or enabled animations for DocViewer transitions, etc.). This commit restores the previous behavior by ensuring that (on the initial rendering) the SideNav state is updated as soon as possible and that there will be no animations when: 1. The hamburger button appears. 2. The SideNav is opened. 3. The main section's width is adjusted to make room for the SideNav. PR Close angular#21695

PR Close angular#21695

During the initial load of the page (probably until the icon styles are loaded and/or applied), the `.header-link` element is wider, pushing the heading text slightly to the right (for a brief moment). This commit prevents this slight shift by explicitly setting the width for the `.header-link` element. PR Close angular#21695

@Maxkorz

…lar#21695) Implemented @Maxkorz's [suggestion](angular#21515 (comment)). Fixes angular#21515 PR Close angular#21695

angular-automatic-lock-bot · 2019-09-13T07:21:02Z

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.}

gkalpak requested a review from petebacondarwin January 22, 2018 11:10

gkalpak added action: review The PR is still awaiting reviews from at least one requested reviewer comp: aio target: patch This PR is targeted for the next patch release labels Jan 22, 2018

googlebot added the cla: yes label Jan 22, 2018

petebacondarwin added this to REVIEW in docs-infra Jan 22, 2018

petebacondarwin reviewed Jan 31, 2018

View reviewed changes

aio/content/navigation.json

@@ -74,7 +74,6 @@

},

{

"url": "tutorial",

Copy link

Member

petebacondarwin Jan 31, 2018

Choose a reason for hiding this comment

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

❤️

petebacondarwin reviewed Jan 31, 2018

View reviewed changes

petebacondarwin approved these changes Jan 31, 2018

View reviewed changes

petebacondarwin added 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 Jan 31, 2018

matsko approved these changes Jan 31, 2018

View reviewed changes

gkalpak force-pushed the fix-aio-minor-fixes branch from 17bbc2f to 4d1ec60 Compare February 1, 2018 12:33

mhevery pushed a commit that referenced this pull request Feb 7, 2018

fix(aio): ignore .header-link when selecting the heading text (#21695)

9d02db3

Implemented @Maxkorz's [suggestion](#21515 (comment)). Fixes #21515 PR Close #21695

mhevery closed this in 8f60473 Feb 7, 2018

mhevery pushed a commit that referenced this pull request Feb 7, 2018

refactor(aio): simplify .header-link styles (#21695)

94d769d

PR Close #21695

mhevery pushed a commit that referenced this pull request Feb 7, 2018

fix(aio): ensure header-links are visible at <600px (#21695)

eb3bfc2

PR Close #21695

mhevery pushed a commit that referenced this pull request Feb 7, 2018

fix(aio): ignore .header-link when selecting the heading text (#21695)

7e95802

Implemented @Maxkorz's [suggestion](#21515 (comment)). Fixes #21515 PR Close #21695

petebacondarwin removed this from MERGE in docs-infra Feb 7, 2018

gkalpak deleted the fix-aio-minor-fixes branch February 8, 2018 08:04

jbogarthyde pushed a commit to jbogarthyde/angular that referenced this pull request Feb 23, 2018

docs(animations): fix typo (disbled --> disabled) (angular#21695)

c1ba55c

PR Close angular#21695

jbogarthyde pushed a commit to jbogarthyde/angular that referenced this pull request Feb 23, 2018

refactor(aio): simplify .header-link styles (angular#21695)

8994f1e

PR Close angular#21695

jbogarthyde pushed a commit to jbogarthyde/angular that referenced this pull request Feb 23, 2018

fix(aio): ensure header-links are visible at <600px (angular#21695)

157fda6

PR Close angular#21695

leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018

docs(animations): fix typo (disbled --> disabled) (angular#21695)

d793aab

PR Close angular#21695

leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018

refactor(aio): simplify .header-link styles (angular#21695)

09e11bd

PR Close angular#21695

leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018

fix(aio): ensure header-links are visible at <600px (angular#21695)

d1247e3

PR Close angular#21695

leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018

fix(aio): ignore .header-link when selecting the heading text (angu…

7392842

…lar#21695) Implemented @Maxkorz's [suggestion](angular#21515 (comment)). Fixes angular#21515 PR Close angular#21695

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix aio minor fixes #21695

Fix aio minor fixes #21695

gkalpak commented Jan 22, 2018 •

edited

mary-poppins commented Jan 22, 2018

petebacondarwin Jan 31, 2018

petebacondarwin Jan 31, 2018

petebacondarwin Jan 31, 2018 •

edited

gkalpak Feb 1, 2018 •

edited

petebacondarwin Feb 1, 2018

gkalpak Feb 1, 2018

petebacondarwin Feb 1, 2018

petebacondarwin Jan 31, 2018

petebacondarwin Jan 31, 2018

gkalpak Feb 1, 2018

petebacondarwin Jan 31, 2018

petebacondarwin Jan 31, 2018

petebacondarwin Jan 31, 2018

petebacondarwin Jan 31, 2018

gkalpak Feb 1, 2018

petebacondarwin Jan 31, 2018

gkalpak Feb 1, 2018

petebacondarwin left a comment

mary-poppins commented Feb 1, 2018

gkalpak commented Feb 1, 2018

gkalpak commented Feb 1, 2018

mary-poppins commented Feb 1, 2018

gkalpak commented Feb 1, 2018

petebacondarwin commented Feb 1, 2018

petebacondarwin commented Feb 1, 2018

petebacondarwin commented Feb 1, 2018

gkalpak commented Feb 1, 2018

mary-poppins commented Feb 1, 2018

angular-automatic-lock-bot bot commented Sep 13, 2019

Fix aio minor fixes #21695

Fix aio minor fixes #21695

Conversation

gkalpak commented Jan 22, 2018 • edited

mary-poppins commented Jan 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin Jan 31, 2018 • edited

Choose a reason for hiding this comment

gkalpak Feb 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

mary-poppins commented Feb 1, 2018

gkalpak commented Feb 1, 2018

gkalpak commented Feb 1, 2018

mary-poppins commented Feb 1, 2018

gkalpak commented Feb 1, 2018

petebacondarwin commented Feb 1, 2018

petebacondarwin commented Feb 1, 2018

petebacondarwin commented Feb 1, 2018

gkalpak commented Feb 1, 2018

mary-poppins commented Feb 1, 2018

angular-automatic-lock-bot bot commented Sep 13, 2019

gkalpak commented Jan 22, 2018 •

edited

petebacondarwin Jan 31, 2018 •

edited

gkalpak Feb 1, 2018 •

edited