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.

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

Report: header, footer, leftnav, share, oh my #2000

Merged
merged 34 commits into from
Apr 28, 2017
Merged
Show file tree
Hide file tree
Changes from 32 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
45b289e
Report header
ebidel Apr 12, 2017
9f8014d
Reponsive styling. Use share icon instead of "Export..."
ebidel Apr 12, 2017
fef3a99
Runtime settings
ebidel Apr 12, 2017
8b4d2e7
Support report re-ender into page
ebidel Apr 12, 2017
7d9e20c
Add report functionality
ebidel Apr 12, 2017
6887af7
Split files
ebidel Apr 12, 2017
2b84229
Remove css focus stuff
ebidel Apr 12, 2017
b138885
Remove placeholders
ebidel Apr 12, 2017
c9633af
Fix tests
ebidel Apr 12, 2017
ee7fcb1
Feedback
ebidel Apr 13, 2017
3b7e90f
lighthouse -> lh
ebidel Apr 13, 2017
c717870
Tests for header/footer
ebidel Apr 13, 2017
85d6b8e
Tests for header/footer/formatter helps
ebidel Apr 13, 2017
2ff34f7
Add more tests
ebidel Apr 13, 2017
e9751c7
Fix rebase
ebidel Apr 13, 2017
2918469
Fix travis tests
ebidel Apr 14, 2017
2a60c3a
Feedback and leftnav
ebidel Apr 15, 2017
121fda3
Add 2nd arg
ebidel Apr 15, 2017
a445a6e
Styling tweaks
ebidel Apr 15, 2017
703043a
Leftnav tests
ebidel Apr 15, 2017
c237657
Disable open in viewer in ui
ebidel Apr 15, 2017
d7f3c29
Adjust passing audits summary
ebidel Apr 15, 2017
aa93acc
Merge branch 'master' into GoogleChrome-tablesclear
patrickhulce Apr 21, 2017
3899e47
Merge after rebase
ebidel Apr 25, 2017
33021e5
Fix up styling after merge
ebidel Apr 25, 2017
31a7385
Update dom usage and reset state of template stamping when exporting …
ebidel Apr 25, 2017
d33cc21
Closure cleanup
ebidel Apr 25, 2017
4bbba21
cleanup
ebidel Apr 26, 2017
a30c86e
Tests
ebidel Apr 26, 2017
d8ce09f
lh-analytics custom event
ebidel Apr 26, 2017
777adfe
fix travis
ebidel Apr 26, 2017
a1ffbf5
Travis fix?
ebidel Apr 26, 2017
e542539
bckenny feedback
ebidel Apr 28, 2017
8bf076f
3 big changes
ebidel Apr 28, 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
1 change: 1 addition & 0 deletions lighthouse-core/closure/closure-type-checking.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ gulp.task('compile-report', () => {
return gulp.src([
// externs
'closure/third_party/commonjs.js',
'closure/third_party/google_universal_analytics_api.js',

'report/v2/renderer/*.js',
])
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
/*
* Copyright 2015 The Closure Compiler Authors.
Copy link
Member

Choose a reason for hiding this comment

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

2015?

*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

/**
* @fileoverview Externs for the Google Universal Analytics API (analytics.js).
*
* The analytics.js API allows the user to choose a name of Google Analytics
* object. This externs file assumes that the name is ‘ga’. If you include
* analytics.js snippet and use a different name, this externs file will not
* be useful to you (unless you change the name ‘ga’ to whatever name you’ve
* chosen).
*
* @see https://developers.google.com/analytics/devguides/collection/analyticsjs/method-reference
* @externs
*/


/**
* @param {string} methodName
* @param {...?} var_args
* @return {?}
*/
function ga(methodName, var_args) {}
Copy link
Member

Choose a reason for hiding this comment

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

I don't think putting functions on this function will actually work. Is the rest of this file needed right now?

Definitely should avoid unknown type and do ...* for var_args and no return type if it's not needed. Right now the second argument is being used as !Object<string, *> and not any more arguments after that. Is that better?

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 took this entire thing from closure's common externs.


/**
* @param {string} trackingId
* @param {!Object=} opt_configObject
* @return {!_GATracker}
*/
ga.create = function(trackingId, opt_configObject) {};

/**
* @param {string} name
* @return {?_GATracker}
*/
ga.getByName = function(name) {};

/** @return {!Array<!_GATracker>} */
ga.getAll = function() {};


/** @type {string|undefined} */
var GoogleAnalyticsObject;


/** @interface */
function _GATracker() {}

/**
* @param {string} hitType
* @param {!Object=} opt_fieldObject
* @return {*}
*/
_GATracker.prototype.send = function(hitType, opt_fieldObject) {};

/**
* @param {string|!Object} nameOrFieldObject
* @param {string|number|!Object|boolean=} opt_value (required if
* nameOrFieldObject is a string)
*/
_GATracker.prototype.set = function(nameOrFieldObject, opt_value) {};

/**
* @param {string} name
* @return {string|number|?Object|boolean}
*/
_GATracker.prototype.get = function(name) {};
32 changes: 1 addition & 31 deletions lighthouse-core/report/v2/renderer/category-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,37 +15,7 @@
*/
'use strict';

/* globals self */

const RATINGS = {
PASS: {label: 'pass', minScore: 75},
AVERAGE: {label: 'average', minScore: 45},
FAIL: {label: 'fail'}
};

/**
* Convert a score to a rating label.
* @param {number} score
* @return {string}
*/
function calculateRating(score) {
let rating = RATINGS.FAIL.label;
if (score >= RATINGS.PASS.minScore) {
rating = RATINGS.PASS.label;
} else if (score >= RATINGS.AVERAGE.minScore) {
rating = RATINGS.AVERAGE.label;
}
return rating;
}

/**
* Format number.
* @param {number} number
* @return {string}
*/
function formatNumber(number) {
return number.toLocaleString(undefined, {maximumFractionDigits: 1});
}
/* globals self, formatNumber, calculateRating */
Copy link
Member

Choose a reason for hiding this comment

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

this global/module.exports thing is increasingly difficult to follow. If the export is just for jsdom and tests, I wonder if we should just do a proxied require for testing the report


class CategoryRenderer {
/**
Expand Down
6 changes: 3 additions & 3 deletions lighthouse-core/report/v2/renderer/dom.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ class DOM {

/**
* @param {string} selector
* @param {!Document|!Element} context
* @param {!Document|!DocumentFragment|!Element} context
Copy link
Member

Choose a reason for hiding this comment

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

should we just switch this to {!Node}?

* @return {!DocumentFragment} A clone of the template content.
* @throws {Error}
*/
Expand Down Expand Up @@ -113,7 +113,7 @@ class DOM {
* Guaranteed context.querySelector. Always returns an element or throws if
* nothing matches query.
* @param {string} query
* @param {!DocumentFragment|!Element} context
* @param {!Document|!DocumentFragment|!Element} context
Copy link
Member

Choose a reason for hiding this comment

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

would {!Node} work? (and for findAll, below)

* @return {!Element}
*/
find(query, context) {
Expand All @@ -127,7 +127,7 @@ class DOM {
/**
* Helper for context.querySelectorAll. Returns an Array instead of a NodeList.
* @param {string} query
* @param {!DocumentFragment|!Element} context
* @param {!Document|!DocumentFragment|!Element} context
* @return {!Array<Element>}
Copy link
Member

Choose a reason for hiding this comment

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

should return !Array<!Element>

*/
findAll(query, context) {
Expand Down
76 changes: 76 additions & 0 deletions lighthouse-core/report/v2/renderer/format-helpers.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
/**
* Copyright 2017 Google Inc. All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
'use strict';

const RATINGS = {
PASS: {label: 'pass', minScore: 75},
AVERAGE: {label: 'average', minScore: 45},
FAIL: {label: 'fail'}
};

/**
* Convert a score to a rating label.
* @param {number} score
* @return {string}
*/
function calculateRating(score) {
let rating = RATINGS.FAIL.label;
if (score >= RATINGS.PASS.minScore) {
rating = RATINGS.PASS.label;
} else if (score >= RATINGS.AVERAGE.minScore) {
rating = RATINGS.AVERAGE.label;
}
return rating;
}

/**
* Format number.
* @param {number} number
* @return {string}
*/
function formatNumber(number) {
return number.toLocaleString(undefined, {maximumFractionDigits: 1});
}

/**
* Format time.
* @param {string} date
* @return {string}
*/
function formatDateTime(date) {
const options = {
month: 'short', day: 'numeric', year: 'numeric',
hour: 'numeric', minute: 'numeric', timeZoneName: 'short'
};
let formatter = new Intl.DateTimeFormat('en-US', options);

// Force UTC if runtime timezone could not be detected.
// See https://github.com/GoogleChrome/lighthouse/issues/1056
const tz = formatter.resolvedOptions().timeZone;
if (!tz || tz.toLowerCase() === 'etc/unknown') {
options.timeZone = 'UTC';
formatter = new Intl.DateTimeFormat('en-US', options);
}
return formatter.format(new Date(date));
}

if (typeof module !== 'undefined' && module.exports) {
module.exports = {
Copy link
Member

Choose a reason for hiding this comment

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

we need to get #2050 landed to get this automatically checked, but I believe this breaks @paulirish's build script. Unless that was reverted? I forget where that landed

Copy link
Member

Choose a reason for hiding this comment

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

also we need to update the replace(/^\s\smodule\.exports = \w+;$/gm, ';') line in the compile script to remove this. This is only passing the compile step because it's currently the only file writing to module.exports because all the rest are removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, even when I uncomment that line the compilation succeeds. Although I don't seem to get a closure-output.js file

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, even when I uncomment that line the compilation succeeds

er, you're right. I guess ignore this :) not sure what changed

Although I don't seem to get a closure-output.js file

you have to set PRINT_CODE to true to get the output. Normally it's just checking, not outputting the compiled code

calculateRating,
formatNumber,
formatDateTime,
};
}
79 changes: 79 additions & 0 deletions lighthouse-core/report/v2/renderer/logger.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
/**
Copy link
Member

Choose a reason for hiding this comment

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

does devtools want a logger? This PR might need to adjust to the model in #2002

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IDK. I'm just porting :)

Knowing the separation of LH proper and integrations of LH is still unclear. Right now the operating assumption is that we're keeping everything we currently have b/c #1937 doesn't capture these types of details.

For me, the dream is that there will be all sorts of existing tools that adopt a LH report. It's impossible to foresee all those integration touch points. Rather than designing our system around a particular integration , it might be easier to flip it. IOW, the integration (e.g. devtools) decides what parts of LH it wants to throw away or replace. Removing handlebars, adding templateContext are a step towards that.

For the features stuff, I've updated it to take the approach in b12f2ea. Devtools can decide not to load the feature (including logger).


Some open thoughts/questions for everyone. "Integration X" can be substituted for "devtools".

  • The core report should contain the features we want to show up in the most places. For example, if you run LH in the CLI/extension/WPT/Gulliver/LHaaS, you'll get the UI we vend (logger, export, etc.).
  • What happens if you generate an html report in Integration X and open it later? Do you see LH's base UI or their version?
  • What happens if you open that same report in Viewer?
  • CSS is the easiest way to get rid of base UI: #lh-log { display: none}. We're already doing that today.
    • Alternatively, Integration X loads its own stylesheets and effectively changes the entire report.
  • If you also want to nuke code, don't inject ReportUIFeature (here), or stub files: class Logger { log() {} }. We could even provide those files for people if it makes sense.

* @license
* Copyright 2017 Google Inc. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

'use strict';

/**
* Logs messages via a UI butter.
* @class
Copy link
Member

Choose a reason for hiding this comment

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

don't need @class FWIW

*/
class Logger {
/**
* @param {!Element} element
*/
constructor(element) {
this.el = element;
Copy link
Member

Choose a reason for hiding this comment

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

should type this until we get NTI: /** @private {!Element} */

/** @private {?number} */
this._id = null;
}

/**
* Shows a butter bar.
* @param {!string} msg The message to show.
* @param {boolean=} optAutoHide True to hide the message after a duration.
* Default is true.
*/
log(msg, optAutoHide) {
Copy link
Member

Choose a reason for hiding this comment

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

we use default params elsewhere already...want to use one here?

const autoHide = typeof optAutoHide === 'undefined' ? true : optAutoHide;

clearTimeout(this._id);

this.el.textContent = msg;
this.el.classList.add('show');
if (autoHide) {
this._id = setTimeout(_ => {
this.el.classList.remove('show');
}, 7000);
}
}

/**
* @param {string} msg
*/
warn(msg) {
Copy link
Member

Choose a reason for hiding this comment

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

need jsdoc on these

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

this.log('Warning: ' + msg);
}

/**
* @param {string} msg
*/
error(msg) {
this.log(msg);
}

/**
* Explicitly hides the butter bar.
*/
hide() {
clearTimeout(this._id);
this.el.classList.remove('show');
}
}

if (typeof module !== 'undefined' && module.exports) {
module.exports = Logger;
}
Loading