feat(aio): add prettier network fail page by wardbell · Pull Request #16139 · angular/angular

wardbell · 2017-04-19T00:39:09Z

When server or network goes down, we serve a canned page. Current ugly page is described in issue #16112.

This PR provides a prettier page along the lines of the better looking 404 page.
It does NOT cache the page so that, once connectivity is restored, it will try again to get that page.

I found a free “network crashed” svg that’s tiny. I’m loading it in index.html for the obvious reason that it needs to exist when you crash. It should go in the service worker pwa-manifest.json too but I don’t know how to do that.

I would have just put it inline but I don’t know how to position and scale an svg. Maybe this is much ado about too little and we should just use a Material Design Icon.

Update (2nd commit)

fixed DocumentService test I broke (refactored for clarity)
switched from svg to Material Design icon for lighter weight and fewer complications
generalized for non-network fetch error causes.

See it work

Open the Mary Poppins preview
Nav to Docs
Pick a guide doc
Open dev tools, network tab
Click "offline"
Nav to a different guide page; should see the new error page
Nav back to prev. page; it should still display perfectly
Go back online
Nav to the formerly-failed guide page; it loads this time.

Todo

@sjtrimble to fix styling.

alfaproject · 2017-04-19T05:41:01Z

aio/src/app/documents/document.service.ts

mary-poppins · 2017-04-19T06:28:44Z

The angular.io preview for daa7d40910fcd57ff0c53b28765ff0c289c5b669 is available here.

mary-poppins · 2017-04-19T06:30:04Z

The angular.io preview for 739d3188567261115810ba7cddf79f7678891751 is available here.

mary-poppins · 2017-04-19T06:32:46Z

The angular.io preview for 5d8879e696ed706f5c5d33b328b7b5c78bf8bd27 is available here.

mary-poppins · 2017-04-19T06:34:08Z

The angular.io preview for 71fa91d5610a517c4ddad913e4c473ac94d22c47 is available here.

gkalpak

LGTM 👍

_{Not related to this PR, but I wish we would use consistent names for variables 😛}

petebacondarwin · 2017-04-19T10:45:35Z

aio/src/app/documents/document.service.ts

Only adding to the cache after the document arrives means that multiple hits to this URL will cause multiple requests until one of them responds.
We should add the observable to the cache when the request is made but then remove it from the cache if the doc that is returned should not be cached.

Something like:

this.cache.set(path, this.fetchDocument(path).do(doc => { if (doc.nocache) { this.cache.remove(path); } });

Good idea .

IgorMinar · 2017-04-19T13:49:43Z

aio/src/index.html

what does async attribute do here? images are loaded in an asynchronous fashion by default.

IgorMinar · 2017-04-19T13:59:31Z

aio/src/app/documents/document.service.ts

IgorMinar · 2017-04-19T14:00:38Z

aio/src/app/documents/document.service.ts

comma missing before "but"?

This text is confusing. How about something shorter and more to the point like: We are sorry, but due to a network issue we can't load document "{{doc}}". Please check your connection and try again.

Because the error isn't always a network issue. It arises from bad JSON, server down (which is not a network connection), and who knows what else.

The rules for commas and conjunctions, especially "but", are all over the place. Some say add it. Some say never! My research and experience say "no comma" in this place but reasonable minds may differ (no comma in that one :)).

Better is to drop the apology.

We can drop the icon altogether too. I just added one because the 404 page had one.

IgorMinar · 2017-04-19T14:07:41Z

aio/src/app/documents/document.service.ts

What's the purpose of "error_outline" text in here? I've seen you use this kind of text elsewhere and it's this text that is causing the flicker. Please remove.

I'm using them exactly as prescribed by the Material Design Guidelines, e.g.,

<i class="material-icons">face</i>

In this case, flicker is highly improbably because there is almost no way to get to this page before the font loads. Therefore, I'm keeping it.

But we need a broader strategy for using MD icons. That should be a separate issue.

Interesting, I didn't know that MD fonts were using ligatures. In that case we must ensure that the font is loaded before we render anything that depends on it.

IgorMinar · 2017-04-19T14:20:38Z

aio/src/app/documents/document.service.ts

why is this page implemented differently than the file not found page? you could implement in the same way as 404 and then use service worker and/or <link rel="preload" to cache it. (https://css-tricks.com/prefetching-preloading-prebrowsing/)

Reasons:

This is the ultimate error fallback. It should never fail. This approach is guaranteed to work even when there is no network (assuming that you've loaded the JS of course).

Service worker is not enabled on most devices.

MDN is not clear on browser support for prefetch.

Why worry? If we're going to get it anyway, and we inline templates routinely, what's wrong with doing it here? Seems simple and sure to me.

IgorMinar · 2017-04-19T14:25:23Z

We also need a different design for this page. This is way too scary and bold.

Among other things the fact that the header is not aligned along with the icon makes it look quite bad.

Current screenshot:

sjtrimble · 2017-04-19T19:15:32Z

Take a look at aio/src/styles/1-layouts/_not-found.scss and aio/content/file-not-found.md from https://github.com/angular/angular/pull/16089/files if you want to take on styling. It would be good to use the same exact classes and format so those types of pages are consistent. Otherwise, I can handle once the new issue is filed :)

wardbell · 2017-04-19T21:06:10Z

Updates address the critical objections raised by reviewers.

Stef and I will work on the remaining cosmetic issues in a separate PR.

@petebacondarwin Please note that the entry in the cache for a document that 404s now has the url of the page that was sought rather than the url of the 404 page that we present. Revised tests reflect this coincidental change.

petebacondarwin · 2017-04-20T07:38:36Z

@petebacondarwin Please note that the entry in the cache for a document that 404s now has the url of the page that was sought rather than the url of the 404 page that we present. Revised tests reflect this coincidental change.

No! 😱 That was the whole point of my change previously! We need to be able to assign the pageId in the AppComponent based on the document being rendered not on the current URL. The test was there on purpose and not incidentally incorrect. I think that calling it URL was a red herring that has confused matters.

mary-poppins · 2017-04-20T15:16:24Z

The angular.io preview for 23b57aa35faf217d449b11b3deb5bca02160c59c is available here.

I made changes to this so my review is no longer valid :-)

mary-poppins · 2017-04-20T15:33:18Z

The angular.io preview for 1925d49 is available here.

mary-poppins · 2017-04-20T15:33:22Z

The angular.io preview for 9f14ce5 is available here.

wardbell

Looks great, Pete. Ship it!

This enables things like embedded components easy access to a clean version of the current location path.

)

) Dgeni is now providing the `id` for all the documents to be viewed. So we no longer need to add this to the DocumentContents object. There are some notable changes in the refactoring: `DocumentService`: * The id of the document to render is now obtained from `LocationService.path()`. * The `getFileNotFoundDoc` and `getErrorDoc` methods have been extracted from the `fetchDocument` method. `AppComponent`: * the `pageId` is now computed in a separate `setPageId` method. `AppComponen` spec file: * The `TestHttp` has had the hard coded documents removed and replaced with a function that will generate docs as needed.

mhevery · 2017-04-21T03:19:03Z

closed with 9730351

This enables things like embedded components easy access to a clean version of the current location path.

…ular#16139)

…ular#16139) Dgeni is now providing the `id` for all the documents to be viewed. So we no longer need to add this to the DocumentContents object. There are some notable changes in the refactoring: `DocumentService`: * The id of the document to render is now obtained from `LocationService.path()`. * The `getFileNotFoundDoc` and `getErrorDoc` methods have been extracted from the `fetchDocument` method. `AppComponent`: * the `pageId` is now computed in a separate `setPageId` method. `AppComponen` spec file: * The `TestHttp` has had the hard coded documents removed and replaced with a function that will generate docs as needed.

This enables things like embedded components easy access to a clean version of the current location path.

…ular#16139)

…ular#16139) Dgeni is now providing the `id` for all the documents to be viewed. So we no longer need to add this to the DocumentContents object. There are some notable changes in the refactoring: `DocumentService`: * The id of the document to render is now obtained from `LocationService.path()`. * The `getFileNotFoundDoc` and `getErrorDoc` methods have been extracted from the `fetchDocument` method. `AppComponent`: * the `pageId` is now computed in a separate `setPageId` method. `AppComponen` spec file: * The `TestHttp` has had the hard coded documents removed and replaced with a function that will generate docs as needed.

angular-automatic-lock-bot · 2019-09-11T09:22:20Z

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

wardbell added the comp: aio label Apr 19, 2017

wardbell self-assigned this Apr 19, 2017

wardbell requested a review from sjtrimble April 19, 2017 00:39

googlebot added the cla: yes label Apr 19, 2017

alfaproject reviewed Apr 19, 2017

View reviewed changes

aio/src/app/documents/document.service.ts Outdated

Copy link
Copy Markdown

Contributor

alfaproject Apr 19, 2017

Choose a reason for hiding this comment

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

revert

wardbell force-pushed the aio-network-fail-page branch 4 times, most recently from 5d8879e to 71fa91d Compare April 19, 2017 06:21

wardbell added the action: review The PR is still awaiting reviews from at least one requested reviewer label Apr 19, 2017

wardbell mentioned this pull request Apr 19, 2017

feat(aio): 404 page #16089

Closed

3 tasks

gkalpak approved these changes Apr 19, 2017

View reviewed changes

petebacondarwin previously requested changes Apr 19, 2017

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 Apr 19, 2017

IgorMinar suggested changes Apr 19, 2017

View reviewed changes

wardbell force-pushed the aio-network-fail-page branch from 71fa91d to acbd486 Compare April 19, 2017 21:43

wardbell added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Apr 19, 2017

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 Apr 20, 2017

petebacondarwin force-pushed the aio-network-fail-page branch from acbd486 to 23b57aa Compare April 20, 2017 14:49

googlebot added the cla: no label Apr 20, 2017

feat(aio): add prettier network fail page

1925d49

IgorMinar approved these changes Apr 20, 2017

View reviewed changes

IgorMinar added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Apr 20, 2017

wardbell commented Apr 20, 2017

View reviewed changes

mhevery pushed a commit that referenced this pull request Apr 21, 2017

feat(aio): add LocationService.path() method (#16139)

6ed812f

This enables things like embedded components easy access to a clean version of the current location path.

mhevery pushed a commit that referenced this pull request Apr 21, 2017

build(aio): add the id to the DocumentContents at doc-gen time (#16139

d0dcabd

)

mhevery pushed a commit that referenced this pull request Apr 21, 2017

feat(aio): add <current-location> embedded component (#16139)

db4e9ea

mhevery pushed a commit that referenced this pull request Apr 21, 2017

feat(aio): add prettier network fail page (#16139)

c390b06

mhevery closed this Apr 21, 2017

wardbell deleted the aio-network-fail-page branch April 21, 2017 18:33

asnowwolf pushed a commit to asnowwolf/angular that referenced this pull request Aug 11, 2017

feat(aio): add LocationService.path() method (angular#16139)

3b839a4

This enables things like embedded components easy access to a clean version of the current location path.

asnowwolf pushed a commit to asnowwolf/angular that referenced this pull request Aug 11, 2017

build(aio): add the id to the DocumentContents at doc-gen time (ang…

8571f15

…ular#16139)

asnowwolf pushed a commit to asnowwolf/angular that referenced this pull request Aug 11, 2017

feat(aio): add <current-location> embedded component (angular#16139)

97dfb3a

asnowwolf pushed a commit to asnowwolf/angular that referenced this pull request Aug 11, 2017

feat(aio): add prettier network fail page (angular#16139)

1198d1e

juleskremer pushed a commit to juleskremer/angular that referenced this pull request Aug 28, 2017

feat(aio): add LocationService.path() method (angular#16139)

ec657a9

This enables things like embedded components easy access to a clean version of the current location path.

juleskremer pushed a commit to juleskremer/angular that referenced this pull request Aug 28, 2017

build(aio): add the id to the DocumentContents at doc-gen time (ang…

a161c1a

…ular#16139)

juleskremer pushed a commit to juleskremer/angular that referenced this pull request Aug 28, 2017

feat(aio): add <current-location> embedded component (angular#16139)

1a92417

juleskremer pushed a commit to juleskremer/angular that referenced this pull request Aug 28, 2017

feat(aio): add prettier network fail page (angular#16139)

939951c

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

Conversation

wardbell commented Apr 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Update (2nd commit)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mary-poppins commented Apr 19, 2017

Uh oh!

mary-poppins commented Apr 19, 2017

Uh oh!

mary-poppins commented Apr 19, 2017

Uh oh!

mary-poppins commented Apr 19, 2017

Uh oh!

gkalpak left a comment

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 Apr 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 Apr 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

wardbell Apr 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IgorMinar commented Apr 19, 2017

Uh oh!

sjtrimble commented Apr 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wardbell commented Apr 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

petebacondarwin commented Apr 20, 2017

Uh oh!

mary-poppins commented Apr 20, 2017

Uh oh!

mary-poppins commented Apr 20, 2017

Uh oh!

mary-poppins commented Apr 20, 2017

Uh oh!

wardbell left a comment

Choose a reason for hiding this comment

Uh oh!

mhevery commented Apr 21, 2017

Uh oh!

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

Uh oh!

Reviewers

Assignees

Labels

wardbell commented Apr 19, 2017 •

edited

Loading

wardbell Apr 19, 2017 •

edited

Loading

wardbell Apr 19, 2017 •

edited

Loading

wardbell Apr 19, 2017 •

edited

Loading

sjtrimble commented Apr 19, 2017 •

edited

Loading

wardbell commented Apr 19, 2017 •

edited

Loading