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

Fixed FOUC #166

Merged
merged 16 commits into from
Jun 1, 2017
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/sky-pages-module-generator.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,13 @@ function getSource(skyAppConfig) {
'SkyAppBootstrapper',
'SkyAppConfig',
'SkyAppWindowRef',
'SkyAppStyleLoader',
'SkyAuthTokenProvider'
];

let runtimeProviders = [
'SkyAppWindowRef',
'SkyAppStyleLoader',
'SkyAuthTokenProvider',
`{
provide: SkyAppConfig,
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
"@blackbaud/auth-client": "1.6.0",
"@blackbaud/help-client": "1.0.1",
"@ngtools/webpack": "1.2.14",
"@types/fontfaceobserver": "0.0.5",
"@types/jasmine": "2.5.40",
"@types/node": "7.0.14",
"angular2-template-loader": "0.5.0",
Expand Down
1 change: 1 addition & 0 deletions runtime/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,4 @@ export * from './bootstrapper';
export * from './config';
export * from './search-results-provider';
export * from './window-ref';
export * from './style-loader';
50 changes: 50 additions & 0 deletions runtime/style-loader.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/* tslint:disable:no-console */

import { SkyAppStyleLoader } from './style-loader';
import * as FontFaceObserver from 'fontfaceobserver';

describe('Style loader', () => {
it('should resolve a promise after loading fonts', (done) => {
spyOn(FontFaceObserver.prototype, 'load').and.returnValue(
Promise.resolve()
);

const styleLoader = new SkyAppStyleLoader();

styleLoader.loadStyles()
.then(() => {
expect(styleLoader.isLoaded).toBe(true);
done();
});
});

it('should pass errors to the resolve', (done) => {
spyOn(Promise, 'all').and.callFake(() => {
return Promise.reject(new Error('Fonts not loaded.'));
});

const styleLoader = new SkyAppStyleLoader();

styleLoader.loadStyles()
.then((response) => {
expect(response.error.message).toBe('Fonts not loaded.');
done();
});
});

it('should resolve the promise if the fonts are not loaded within the timeout', (done) => {
const sampleObserver = new FontFaceObserver('SampleFont');

spyOn(Promise, 'all').and.callFake(() => {
return sampleObserver.load(undefined, 1);
});

const styleLoader = new SkyAppStyleLoader();

styleLoader.loadStyles()
.then((result) => {
expect(result.error).toBeDefined();
done();
});
});
});
38 changes: 38 additions & 0 deletions runtime/style-loader.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import * as FontFaceObserver from 'fontfaceobserver';

import { Injectable } from '@angular/core';

@Injectable()
export class SkyAppStyleLoader {
public static readonly LOAD_TIMEOUT: number = 3000;
public isLoaded: boolean = false;

public loadStyles(): Promise<any> {
const fontAwesome = new FontFaceObserver('FontAwesome');
const openSans = new FontFaceObserver('Open Sans');
const oswald = new FontFaceObserver('Oswald');

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;
return Promise.resolve({
status: true

Choose a reason for hiding this comment

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

Hey @Blackbaud-SteveBrush is status used somewhere else?

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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
  }
});

Choose a reason for hiding this comment

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

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
        });
      });

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

Choose a reason for hiding this comment

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

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>

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

});
});
}
}
5 changes: 4 additions & 1 deletion src/app/app.component.html
Original file line number Diff line number Diff line change
@@ -1 +1,4 @@
<router-outlet></router-outlet>
<div [ngClass]="{ 'skyux-app-loading': !isReady }">
<router-outlet>
</router-outlet>
</div>
3 changes: 3 additions & 0 deletions src/app/app.component.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
.skyux-app-loading {
visibility: hidden;
}
17 changes: 15 additions & 2 deletions src/app/app.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ import { BBHelp } from '@blackbaud/help-client';
import {
SkyAppConfig,
SkyAppSearchResultsProvider,
SkyAppWindowRef
SkyAppWindowRef,
SkyAppStyleLoader
} from '@blackbaud/skyux-builder/runtime';

require('style-loader!@blackbaud/skyux/dist/css/sky.css');
Expand Down Expand Up @@ -70,12 +71,24 @@ function fixUpNav(nav: any, baseUrl: string) {
templateUrl: './app.component.html'
})
export class AppComponent implements OnInit {
public isReady = false;

constructor(
private router: Router,
private windowRef: SkyAppWindowRef,
private config: SkyAppConfig,
private styleLoader: SkyAppStyleLoader,
@Optional() private searchProvider?: SkyAppSearchResultsProvider
) { }
) {
this.styleLoader.loadStyles()
.then((result) => {
this.isReady = true;

if (result.error) {
console.log(result.error.message);
}
});
}

public ngOnInit() {
// Without this code, navigating to a new route doesn't cause the window to be
Expand Down