Fixed FOUC #166

Blackbaud-SteveBrush · 2017-05-25T18:13:34Z

Addresses issue 707 and 157

codecov-io · 2017-05-25T18:24:21Z

Codecov Report

Merging #166 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #166   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          34     35    +1     
  Lines         720    734   +14     
  Branches       91     91           
=====================================
+ Hits          720    734   +14

Flag	Coverage Δ
#builder	`100% <ø> (ø)`	⬆️
#runtime	`100% <100%> (ø)`	⬆️

Impacted Files	Coverage Δ
lib/sky-pages-module-generator.js	`100% <ø> (ø)`	⬆️
runtime/style-loader.ts	`100% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update babe3f1...9c2c024. Read the comment docs.

Blackbaud-BobbyEarl · 2017-05-26T13:51:54Z

tsconfig.json

-    "types": [
-      "jasmine",
-      "node"
+    "typeRoots": [


Any insight into what this change does?

It allows tsc to find types for fontfaceobserver. I could be deliberate and add "fontfaceobserver" to the list, if that's preferred.

Ok, looking at this a little more. Reading over the "@types, typeRoots and types" section of https://www.typescriptlang.org/docs/handbook/tsconfig-json.html, it seems you've put back in what is the default behavior.

Since you're importing fontFaceObjserver using the module syntax, I would've expected this sentence to be true.

"Keep in mind that automatic inclusion is only important if you’re using files with global declarations (as opposed to files declared as modules). If you use an import "foo" statement, for instance, TypeScript may still look through node_modules & node_modules/@types folders to find the foo package."

Removing this change from this branch as it doesn't seem to be throwing the same error anymore. I guess I fixed it inadvertently.

Blackbaud-BobbyEarl · 2017-05-26T13:52:54Z

src/app/app.component.html

@@ -1 +1,4 @@
-<router-outlet></router-outlet>
+<div class="sky-pages-cloak" [ngClass]="{ 'visible': isReady }">


How about skyux-spa-cloak?

Yeah, I can do that. I was mimicking the id of the app's wrapping div.

Ahh I see. Ignore my comment. :-)

I'd rather see an ngClass attribute that adds a CSS class when !isReady so that it's removed when isReady is true. That way you only need one CSS class instead of two. As for the name, I'd use something like skyux-app-loading.

Blackbaud-BobbyEarl · 2017-05-26T13:57:37Z

runtime/style-loader.ts

+        // Errors loading the font should not stop the page from rendering.
+        // Passing the error along in case the client wants to do something with it.
+        return Promise.resolve({
+          error: error


Is error ever used? I agree it makes sense to have the app still load in the event of an error, but maybe we could at least console.log it, or even add something to the template?

<div *ngIf="error"> There was an error loading the requested fonts. The application is still available. </div>

Well, the "error" will still fire if the font is taking too long to load (so, it might still exist, but is taking longer than 3 seconds to load). We can console log it if that helps, but thought it would be helpful to provide the error to the consuming service, if they wanted to check for it. Otherwise, there's no way for the consumer to "know" that an error occurred, because it will always be resolved:
https://github.com/blackbaud/skyux-builder/pull/166/files#diff-df8c52ab04ab7a3e6b86e304a41fd060R30

I believe by putting my comment in the wrong spot it's thrown us off. I'm going to comment in the right place.

Blackbaud-BobbyEarl · 2017-05-26T14:32:46Z

src/app/app.component.ts

    @Optional() private searchProvider?: SkyAppSearchResultsProvider
-  ) { }
+  ) {
+    this.styleLoader.loadStyles().then(() => {


From our previous conversation, my suggestion was something like this:

public hasError = false; this.styleLoader.loadStyles.then(error => { this.isReady = true; if (error) { console.log(error); this.hasError = true; } });

And then in app.component.html, something like:

<div *ngIf="hasError"> There was an error loading the requested fonts. The application is still available. </div>

Ah, that makes sense. Do we want to show an error on the front-end, though? What if we put it in the red banner?

The red banner is a implementation in host only, not in builder. We've reserved that to indicate when a site is being served locally.

I'm for visually showing something, but perhaps a console.log is sufficient.

Perhaps @Blackbaud-PaulCrowder has an opinion?

I'd prefer to gracefully fail and not show an error message. Writing something to console.log should be sufficient.

Blackbaud-PaulCrowder · 2017-05-26T16:54:26Z

src/app/app.component.ts

    @Optional() private searchProvider?: SkyAppSearchResultsProvider
-  ) { }
+  ) {
+    this.styleLoader.loadStyles().then(() => {


I'd prefer to gracefully fail and not show an error message. Writing something to console.log should be sufficient.

Blackbaud-PaulCrowder · 2017-05-26T16:57:45Z

src/app/app.component.html

@@ -1 +1,4 @@
-<router-outlet></router-outlet>
+<div class="sky-pages-cloak" [ngClass]="{ 'visible': isReady }">


I'd rather see an ngClass attribute that adds a CSS class when !isReady so that it's removed when isReady is true. That way you only need one CSS class instead of two. As for the name, I'd use something like skyux-app-loading.

Blackbaud-BobbyEarl · 2017-05-30T21:46:21Z

runtime/style-loader.ts

+      .then(() => {
+        this.isLoaded = true;
+        return Promise.resolve({
+          status: true


Hey @Blackbaud-SteveBrush is status used somewhere else?

It's just another way to detect if the resolution was a success (result.error, result.status). It's either this or an empty resolve (we need a resolve either way). What do you think?

Maybe, return Promise.resolve({ error: undefined })? The issue comes when you need to check if result.error exists. If there isn't an error, result.error throws an error because result is undefined.

this.styleLoader.loadStyles().then((result: any) => { if (result.error) { // <-- throws error } });

Thoughts on just making result optional?

this.styleLoader.loadStyles().then((result?: any) => { if (result && result.error) { } });

Then I think you could also remove that unnecessary return Promise.

return Promise .all([ // Specify a character for FontAwesome since some browsers will fail to detect // when the font is loaded unless a known character with a different width // than the default is not specified. fontAwesome.load('\uf0fc', SkyAppStyleLoader.LOAD_TIMEOUT), openSans.load(undefined, SkyAppStyleLoader.LOAD_TIMEOUT), oswald.load(undefined, SkyAppStyleLoader.LOAD_TIMEOUT) ]) .then(() => { this.isLoaded = true; }) .catch((error) => { // Errors loading the font should not stop the page from rendering. // Passing the error along in case the client wants to do something with it. return Promise.resolve({ error: error }); });

Blackbaud-BobbyEarl

LGTM, but @Blackbaud-PaulCrowder had previously requested changes, so I think he'll need to approve as well.

* Updated to release 3.10.0. * Fixed package-lock.json * Added additional entries. Updated date. * Update CHANGELOG.md Minor tweaks * Removed extra parentheses

Blackbaud-SteveBrush added 7 commits May 25, 2017 12:06

Added style loader to runtime

d073071

Added style loader to runtime

81d220a

Created typescript module

05d5f42

Merge branch 'master' into fix-fouc

e7a9c08

Added types

26421e1

Added space in attribute

ae6be64

Reduced timeout

6e01941

Blackbaud-SteveBrush changed the title ~~Fixed FOUC~~ [HOLD] Fixed FOUC May 25, 2017

Added unit tests

40c523e

Blackbaud-SteveBrush changed the title ~~[HOLD] Fixed FOUC~~ Fixed FOUC May 25, 2017

Blackbaud-SteveBrush added 5 commits May 25, 2017 16:19

Fixed linting error

7acc687

Updated skyux version

a11758a

Updated to injectable

b50470e

Implemented zones

8c16bd1

Removed zones

1f1e39d

Blackbaud-SteveBrush requested a review from Blackbaud-BobbyEarl May 26, 2017 02:35

Blackbaud-BobbyEarl reviewed May 26, 2017

View reviewed changes

Blackbaud-PaulCrowder suggested changes May 26, 2017

View reviewed changes

Blackbaud-SteveBrush added 2 commits May 26, 2017 13:18

Changed name of ready class

8a63d79

Returning a resolved promise

6c015f0

Blackbaud-BobbyEarl reviewed May 30, 2017

View reviewed changes

Made result optional

9c2c024

Blackbaud-BobbyEarl approved these changes May 31, 2017

View reviewed changes

Blackbaud-PaulCrowder approved these changes Jun 1, 2017

View reviewed changes

Blackbaud-PaulCrowder merged commit 15a547c into master Jun 1, 2017

Blackbaud-PaulCrowder deleted the fix-fouc branch June 1, 2017 18:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed FOUC #166

Fixed FOUC #166

Blackbaud-SteveBrush commented May 25, 2017 •

edited

Loading

codecov-io commented May 25, 2017 •

edited

Loading

Blackbaud-BobbyEarl May 26, 2017

Blackbaud-SteveBrush May 26, 2017

Blackbaud-BobbyEarl May 26, 2017

Blackbaud-SteveBrush May 26, 2017

Blackbaud-BobbyEarl May 26, 2017

Blackbaud-SteveBrush May 26, 2017

Blackbaud-BobbyEarl May 26, 2017

Blackbaud-PaulCrowder May 26, 2017

Blackbaud-BobbyEarl May 26, 2017

Blackbaud-SteveBrush May 26, 2017

Blackbaud-BobbyEarl May 26, 2017

Blackbaud-BobbyEarl May 26, 2017

Blackbaud-SteveBrush May 26, 2017

Blackbaud-BobbyEarl May 26, 2017

Blackbaud-PaulCrowder May 26, 2017

Blackbaud-PaulCrowder May 26, 2017

Blackbaud-PaulCrowder May 26, 2017

Blackbaud-BobbyEarl May 30, 2017

Blackbaud-SteveBrush May 30, 2017

Blackbaud-SteveBrush May 30, 2017

Blackbaud-BobbyEarl May 31, 2017

Blackbaud-BobbyEarl left a comment

		@@ -1 +1,4 @@
		<router-outlet></router-outlet>
		<div class="sky-pages-cloak" [ngClass]="{ 'visible': isReady }">

Fixed FOUC #166

Fixed FOUC #166

Conversation

Blackbaud-SteveBrush commented May 25, 2017 • edited Loading

codecov-io commented May 25, 2017 • edited Loading

Codecov Report

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

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

Blackbaud-BobbyEarl left a comment

Choose a reason for hiding this comment

Blackbaud-SteveBrush commented May 25, 2017 •

edited

Loading

codecov-io commented May 25, 2017 •

edited

Loading