Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Viewer: add sw for ATHS #1571

Merged
merged 5 commits into from
Jan 31, 2017
Merged

Viewer: add sw for ATHS #1571

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bc1553a
Viewer: add sw for ATHS
ebidel Jan 28, 2017
cf12c9e
feedback
ebidel Jan 30, 2017
f18e654
no scope
ebidel Jan 30, 2017
29d35c4
Register sw on path
ebidel Jan 30, 2017
f5eca33
feedback
ebidel Jan 31, 2017
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: 1 addition & 1 deletion lighthouse-viewer/app/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
<meta name="viewport" content="width=device-width, initial-scale=1, minimum-scale=1">
<title>Lighthouse Report Viewer</title>
<link rel="shortcut icon" href="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACAAAAAgCAIAAAD8GO2jAAADjklEQVR4AWI08P/HQEvAQrxSQKvlECfLFYXx75xCY2qmh89GbNvOMjb3v9jOOlxnFWxj206ebQ3b7q6q+z1rNagu8/zvPSZACAABpeUAA0miMgU7SA7JjCraFGwZwECOwvL75dWjsKgWBKtx0jvWo+vkBAFbACCkByMP6nMn48+AVgXB2fzSCwsv22/lMGlUhmJ0AE7BH8dyUUDbUEgN6RzJRSeaPxhdRYR0Inel+7Hd5lBiFpkMAxACc0394//9C4voFHDiAAGLpuOXebdfdHfctgwJKaZRLRKy6ItrSis6RBnVBgGtbHyKTEmJHQoEXoBCE5BCrDeA2ogMUIGDAKEBDEhUqwgMqBYDjW4DQzmuffVdqff42/ZQYYqVcMXGZsMPyCsH3lyJSetxvEaxAQXdjR1HjfwCdIS7lo2DZke26Qe+MXO12OWkGT0O6oE7vMGkMnkYw4aN1KQgMKExhXqswfiov4+a7MQ11XPnbr/5qpKlgACAAQj94Lu271bN9DUecQasIZlNzG72llRAAKJiAi+/BSHrSFjRvQhg3DEKEqJh08tsmLTx597+f6enr4cc2Zpk57pihfX24dW7RHcOLLUbJYhJSl0ErQCI9BVXH/XrO97QasuvQQSiECa0BrQCIIJp6X9T/r8QG6L71WYSqCoIIGo2BZDUBnS/D9EA9Nun1iYvbM0MFExIDQRoKFatc1Z6zrm5uWeObJotq0BGV9FuQBWq5a4Fw3PPz848rZHstZSuA5FWAFSMP2nOppOOGpl6qh9PCSg0IFyHKjSQyDNQHTru2t75NOEe0fsf246oAmFkI6vCdnWvbQFQFCKx8vCswV8TrDLiDLgH4Nr7RAtNsrC9d8sfk7b8ls4igdNy8CQKAISlsB0FjZfd3Lfp155tf8fKI4BxZZIj/oTdVEAIAcJFOCmzauHG71I7/rdreUAgAqpDP05fDARCAQQARwEIBQSVxq0FyaLvZZtevpHa8WHw8cft6cpxlq8eAJtIhnSbWDf951yx3y13OqUuu5qyGgkxCgGFh9cDihDGbTa6BqvT1lWmrav3bmt2ZMJ4mU6TGgIC4DBzcv/JqAau1WhzSt3x9Ixk/4Jk/8J4ZrrViFMA4W6A7+WK8xcVjvyrOmVD0FbAXokcT48r+xVqLKvuJYbmpNadnlp3mpufJHOe/GXktM+r09bT8kEdq9BRYAbGSgzP7ll82U71Mc+ZFooXgwAAAABJRU5ErkJggg==">
<link rel="manifest" href="images/manifest.json">
<link rel="manifest" href="manifest.json">
Copy link
Member

Choose a reason for hiding this comment

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

can you throw the manifest link above the favicon reference?
IMO makes sense before, based on priority.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

<meta name="theme-color" content="#304ffe">

<!-- build:css styles/main.css -->
Expand Down
7 changes: 4 additions & 3 deletions lighthouse-viewer/app/images/manifest.json → lighthouse-viewer/app/manifest.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,18 @@
"short_name": "Lighthouse",
"icons": [
{
"src": "android-chrome-192x192.png",
"src": "images/android-chrome-192x192.png",
"sizes": "192x192",
"type": "image/png"
},
{
"src": "android-chrome-512x512.png",
"src": "images/android-chrome-512x512.png",
"sizes": "512x512",
"type": "image/png"
}
],
"theme_color": "#304ffe",
"background_color": "#304ffe",
"display": "standalone"
"display": "standalone",
"start_url": "./"
}
7 changes: 7 additions & 0 deletions lighthouse-viewer/app/src/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,3 +43,10 @@ Promise.all(loadPolyfillPromises).then(_ => {
// eslint-disable-next-line no-new
new LighthouseViewerReport();
});

if ('serviceWorker' in navigator) {
navigator.serviceWorker.register('/sw.js', {scope: './'}).catch(e => {
Copy link
Member

Choose a reason for hiding this comment

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

isn't that scope implied? (iirc, the default scope matches the path of the SW resource?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've been told to ALWAYS use scope. @jeffposnick says so!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

// eslint-disable-next-line no-console
Copy link
Member

Choose a reason for hiding this comment

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

since this file is only for running in the browser, want to just do /* eslint-disable no-console */ at the top of the file? Or is it better to keep it around to cut down on console calls left over from development?

(the rule is a bit annoying for debugging in viewer and in the extension since both have the gulp lint task as a required prerequisite for gulp build, but it's not the end of the world)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just removed the .catch for now. We don't care if this shell sw fails to register.

console.error('Error during service worker registration:', e);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually, we probably want to register this at ${location.pathname}sw.js so it is restricted to googlechrome.github.io/lighthouse/viewer/ and also works on localhost.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

navigator.serviceWorker.register('sw.js') for that matter.

});
}
1 change: 1 addition & 0 deletions lighthouse-viewer/app/sw.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
// Intentionally empty for now.
Copy link
Member

Choose a reason for hiding this comment

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

needs license :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

13 changes: 10 additions & 3 deletions lighthouse-viewer/gulpfile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ function license() {
gulp.task('lint', () => {
return gulp.src([
'app/src/**/*.js',
'gulpfile.js'
'gulpfile.js',
'sw.js'
])
.pipe($.eslint())
.pipe($.eslint.format())
Expand All @@ -65,7 +66,11 @@ gulp.task('concat-css', ['html', 'css'], () => {
});

gulp.task('html', () => {
return gulp.src('app/*.html').pipe(gulp.dest(DIST_FOLDER));
return gulp.src([
'app/*.html',
'app/sw.js',
'app/manifest.json'
]).pipe(gulp.dest(DIST_FOLDER));
});

gulp.task('polyfills', () => {
Expand Down Expand Up @@ -124,7 +129,9 @@ gulp.task('watch', ['lint', 'browserify', 'polyfills', 'html', 'images', 'css'],
});

gulp.watch([
'app/index.html'
'app/index.html',
'app/manifest.json',
'app/sw.js'
]).on('change', () => {
runSequence('html');
});
Expand Down