-
Notifications
You must be signed in to change notification settings - Fork 28
Fixed FOUC #166
Fixed FOUC #166
Changes from 15 commits
d073071
81d220a
05d5f42
e7a9c08
26421e1
ae6be64
6e01941
40c523e
7acc687
a11758a
b50470e
8c16bd1
1f1e39d
8a63d79
6c015f0
9c2c024
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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(); | ||
}); | ||
}); | ||
}); |
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 | ||
}); | ||
}) | ||
.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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
}); | ||
}); | ||
} | ||
} |
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> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
.skyux-app-loading { | ||
visibility: hidden; | ||
} |
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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?There was a problem hiding this comment.
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 ifresult.error
exists. If there isn't an error,result.error
throws an error becauseresult
is undefined.There was a problem hiding this comment.
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?
Then I think you could also remove that unnecessary return Promise.