-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Add table list formatter for HTML audits #1505
Conversation
Niiiiiiiiiiiice, I was needing something like this! |
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.
awesome stuff!
results.push({ | ||
url, | ||
total: `${originalKb} KB`, | ||
webpSavings: `${webpSavings ? webpSavings.percent + '%' : ''}`, |
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.
don't need the check here for webpSavings
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.
done
url, | ||
total: `${originalKb} KB`, | ||
webpSavings: `${webpSavings ? webpSavings.percent + '%' : ''}`, | ||
jpegSavings: `${jpegSavings ? jpegSavings.percent + '%' : ''}` |
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.
either add a check for it to be positive here or move jpegSavings = ...
inside the if above?
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.
probably also move these out of `` strings :)
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.
done
value: groupedResults | ||
formatter: Formatter.SUPPORTED_FORMATS.TABLE, | ||
value: { | ||
results: groupedResults, |
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.
no changes needed to the mapping?
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.
nope. The table formatter will pull from thegroupedResults
object as needed, based on the keys used in the first arg to createTable({url: 'URL', lineCol: 'Line/Col', type: 'Type', code: 'Snippet'})
.
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.
Ah so the extra info was already in there and was being ignored by URLLIST?
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.
Yea.
<style> | ||
.table_list { | ||
margin-top: 8px; | ||
border: 1px solid #EBEBEB; |
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.
do we care about alphabetizing our rules?
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.
There's a PR that alphabetizing selectors was brought up (can't find it now), but we deemed it unrealistic without tooling.
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.
a-ok by me!
* @param {!Object<string, string>} headings for the table. The order of this | ||
* object's key/value will be the order of the table's headings. | ||
* @param {!Array<*>} results Audit results. | ||
* @return {!{headings: string, rows: [{cols: [*]}]}} headings |
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.
nit: no !
needed, headings is Array<string>
isn't it?
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.
thanks. done.
PTAL |
To keep formatting out of the audits (and avoiding the static getHelpers() {
createTable
} to the formatter, then have the extendedInfo: {
formatter: Formatter.SUPPORTED_FORMATS.TABLE,
value: {
results,
tableHeaders
}
} and then have WDYT? |
this is a huge improvement |
|
||
/** | ||
* Preps a formatted table (headings/col vals) for output. | ||
* @param {!Object<string, string>} headings for the table. The order of this |
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.
nit: can just do !Object<string>
(since all object keys are strings)
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.
done
const cols = headingKeys.map(key => { | ||
switch (key) { | ||
case 'code': | ||
// Wrap code snippets in markdown ticks. |
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.
worth documenting these special keys in jsdoc?
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.
done. added to the function description. If we ad more one-offs, that'll be easy to doc more.
* Preps a formatted table (headings/col vals) for output. | ||
* @param {!Object<string, string>} headings for the table. The order of this | ||
* object's key/value will be the order of the table's headings. | ||
* @param {!Array<*>} results Audit results. |
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.
looks like you maybe expect this to be !Array<!Object>
?
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.
done
* @param {!Object<string, string>} headings for the table. The order of this | ||
* object's key/value will be the order of the table's headings. | ||
* @param {!Array<*>} results Audit results. | ||
* @return {{headings: Array<string>, rows: [{cols: [*]}]}} headings |
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 {{headings: !Array<string>, rows: !Array<{cols: !Array<*>}>}}
?
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.
done
PTAL. |
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.
LGTM
┬─┬
R: all
This adds a new
<table>
formatter for HTML reports which increases clarity, fixes some overflow UI bugs we had with text, and will allow us to more easily add additional extended info in the future (e.g. extra properties from the gatherers, RUM data column, etc.)An audit uses the formatter like so:
Notes:
overflow: auto
so you can scroll them to reveal more of the url/code snippetsresults
are also being returned inextendedInfo
so unit testing is easier and so we have the full objects as returned by the gatherer. Might want to change this in the future b/c it does mean the JSON results will be larger than they need to be.After:
Before: