report: new audit list display, indexes & new icons #5109

paulirish · 2018-05-04T00:00:20Z

e.g.

example reports:

https://ambiguous-river.surge.sh/cnn.html
https://ambiguous-river.surge.sh/tinyhouse.html
https://ambiguous-river.surge.sh/dbwtester.html
https://ambiguous-river.surge.sh/example.html
https://ambiguous-river.surge.sh/paulirish.html

https://ambiguous-river.surge.sh/cnn.devtools.html
https://ambiguous-river.surge.sh/tinyhouse.devtools.html
https://ambiguous-river.surge.sh/dbwtester.devtools.html
https://ambiguous-river.surge.sh/example.devtools.html
https://ambiguous-river.surge.sh/paulirish.devtools.html

In a separate PR I will be adjusting the whitespace and alignment of everything.

about to merge displayValue & score together...

patrickhulce · 2018-05-04T00:17:03Z

review on its way, but few UI comments before I forget:

we're planning on bringing the numbering to opportunities too, right? paul says yes
there doesn't seem to be a dropdown indicator for the "Passed audits" and manual audits, I wouldn't expect I'd be able to click on them
I am lovin' display value now-a-days!
Don't think we need to do this immediately, but it'd make more sense IMO if they numbers on the audits continued counting across audit groups in a11y (like how we say the total # of passed audits/not applicable)

vinamratasingal-zz · 2018-05-04T00:19:07Z

+1 to Patrick's comments around adding dropdown arrows to the opportunities + manual audits + passed audits- i think that might impede discoverability.

cc: @hwikyounglee

patrickhulce · 2018-05-04T00:21:25Z

lighthouse-core/report/html/renderer/performance-category-renderer.js

- .filter(audit => audit.group === 'diagnostics' && audit.result.score < 1);
+ .filter(audit => audit.group === 'diagnostics' && audit.result.score < 1)
+ .sort((a, b) => {
+ const scoreA = a.result.informative ? 100 : a.result.score;


just a heads up to line up with #5105 whoever lands first 🐎 🚗 🏁

re:@vinamratasingal's comment on dropdown -- I think once we get the overall left/right alignments on the layout, and the dropdown color being darker as intended, it would look better. Keeping watching out for any issues though!

patrickhulce

lookin' real good 👍

patrickhulce · 2018-05-04T00:24:01Z

lighthouse-core/report/html/report-styles.css

+ --pass-icon-url: url('data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 48 48"><title>check</title><path fill="hsl(139, 70%, 30%)" d="M24 4C12.95 4 4 12.95 4 24c0 11.04 8.95 20 20 20 11.04 0 20-8.96 20-20 0-11.05-8.96-20-20-20zm-4 30L10 24l2.83-2.83L20 28.34l15.17-15.17L38 16 20 34z"/></svg>');
+ --average-icon-url: url('data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 48 48"><title>info</title><path fill="hsl(31, 100%, 45%)" d="M24 4C12.95 4 4 12.95 4 24s8.95 20 20 20 20-8.95 20-20S35.05 4 24 4zm2 30h-4V22h4v12zm0-16h-4v-4h4v4z"/></svg>');
+ --fail-icon-url: url('data:image/svg+xml;utf8,<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 48 48"><title>warn</title><path fill="hsl(1, 73%, 45%)" d="M2 42h44L24 4 2 42zm24-6h-4v-4h4v4zm0-8h-4v-8h4v8z"/></svg>');
+ --collapsed-triangle-icon-url: url('data:image/svg+xml;utf8,<svg width="12" height="12" viewBox="0 0 12 12" xmlns="http://www.w3.org/2000/svg"><g fill="none" fill-rule="evenodd"><path fill="none" d="M0 0h12v12H0z"/><path fill="hsl(0, 0%, 60%)" d="M3 2l6 4-6 4z"/></g></svg>');


what are these? triangle makes me think of the alert/fail icon

it looks like toggle-arrow is the chevron?

maybe we need some comments :)

patrickhulce · 2018-05-04T00:28:07Z

lighthouse-core/report/html/report-styles.css

@@ -462,7 +446,7 @@ summary {

 .lh-load-opportunity__stats {
 text-align: right;
- flex: 0 0 var(--lh-audit-score-width);
+ flex: 0 0 calc(5 * var(--body-font-size));


why the drive by calc now? is the real width not in a var anymore?

just a temporary solution till i take on opportunity display. :)

patrickhulce · 2018-05-04T00:29:40Z

lighthouse-core/report/html/report-styles.css

-.lh-score__value--fail::after {
- background: var(--fail-icon-url) no-repeat center center / 12px 12px;
+.lh-audit--informative .lh-audit__display-text {
+ color: hsl(216, 5%, 39%);


are we going to reuse this color? it's a gray? css var perhaps then :)

about to merge displayValue & score together...

…tdisplay2

paulirish · 2018-05-04T05:18:20Z

feedback addressed. bumped the surge reports, too.

paulirish · 2018-05-04T05:19:50Z

we're planning on bringing the numbering to opportunities too, right?

yes, separate PR.

there doesn't seem to be a dropdown indicator for the "Passed audits" and manual audits, I wouldn't expect I'd be able to click on them

added. and added in the "4 audits" count as well. :)

Don't think we need to do this immediately, but it'd make more sense IMO if they numbers on the audits continued counting across audit groups in a11y (like how we say the total # of passed audits/not applicable)

definitely want to do this. but categoryRenderer doesn't make it easy. so i'll leave it until the file isn't so volatile.

patrickhulce

code LGTM! just some text 🚲 🏠

report's lookin' great!!

patrickhulce · 2018-05-04T15:15:56Z

lighthouse-core/audits/bootup-time.js

- let totalBootupTime = 0;
- /** @type {Object<string, Object<string, number>>} */
- const extendedInfo = {};
+ const devtoolsTimelineModel = await artifacts.requestDevtoolsTimelineModel(trace);


rebase noise?

yah definitely. weird that it shows up. in the diff. I didn't touch these. :/

patrickhulce · 2018-05-04T15:16:43Z

lighthouse-core/report/html/renderer/category-renderer.js

@@ -161,8 +166,8 @@ class CategoryRenderer {
 */
 _renderFailedAuditsSection(elements) {
 const failedElem = this.renderAuditGroup({
- title: `${this._getTotalAuditsLength(elements)} Failed Audits`,
- }, {expandable: false});
+ title: `Failed audits`,


aww I liked them caps'd :(

mocks say no to Title Case. :)
cc @hwikyounglee

patrickhulce · 2018-05-04T15:17:28Z

lighthouse-core/report/html/renderer/category-renderer.js

@@ -187,8 +192,8 @@ class CategoryRenderer {
 */
 _renderNotApplicableAuditsSection(elements) {
 const notApplicableElem = this.renderAuditGroup({
- title: `${this._getTotalAuditsLength(elements)} Not Applicable Audits`,
- }, {expandable: true});
+ title: `Not applicable`,


why make this different from "Passed/Failed audits" we should remove from all if that's what we want, I'm cool with that 👍

cc @hwikyounglee

i was going off the mocks. I don't mind either way, but i do enjoy the brevity.

Good points. Happy for now with the current state. // Being said, we'll soon have a chance to polish the UI strings further (recently started sharing the latest mocks with the UX Writer for advice.

paulirish added 8 commits May 3, 2018 16:27

New audit list display.

35d6e66

about to merge displayValue & score together...

byebye check and x

673950d

rename score__value and improve header styling

3db3eb9

move --pass --fail classes to audit.

1fc2ab5

apply fail color to display-text

d57dc78

fix rebase

32127e6

lint.

9141a8e

sort the diagnostics based on score.

2aba130

paulirish requested a review from vinamratasingal-zz as a code owner May 4, 2018 00:00

paulirish requested a review from patrickhulce May 4, 2018 00:00

paulirish mentioned this pull request May 4, 2018

[meta] 2018 I/O Burndown #5008

Closed

82 tasks

patrickhulce reviewed May 4, 2018

View reviewed changes

Merge branch 'master' into auditlistdisplay2

0381f02

patrickhulce reviewed May 4, 2018

View reviewed changes

brendankenny and others added 13 commits May 3, 2018 21:28

core(displayValue): fancier displayValue type (#5111)

b49e6ce

New audit list display.

dec3af9

about to merge displayValue & score together...

byebye check and x

05ec181

rename score__value and improve header styling

0bd4dd0

move --pass --fail classes to audit.

4e3ae52

apply fail color to display-text

dcb3b6a

fix rebase

37444d4

lint.

550fc58

sort the diagnostics based on score.

7eb9570

show toggle-arrow for groups. add item count for groups

a81416d

Merge remote-tracking branch 'origin/auditlistdisplay2' into auditlis…

17ae280

…tdisplay2

style tweaks.

18df349

icon feedback.

ab12dd5

paulirish requested a review from brendankenny as a code owner May 4, 2018 05:11

patrickhulce approved these changes May 4, 2018

View reviewed changes

paulirish merged commit af87207 into master May 4, 2018

brendankenny deleted the auditlistdisplay2 branch May 4, 2018 18:57

kdzwinel pushed a commit to kdzwinel/lighthouse that referenced this pull request Aug 16, 2018

report: new audit list display, indexes & new icons (GoogleChrome#5109)

dab3e58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

report: new audit list display, indexes & new icons #5109

report: new audit list display, indexes & new icons #5109

paulirish commented May 4, 2018 •

edited

Loading

patrickhulce commented May 4, 2018 •

edited

Loading

vinamratasingal-zz commented May 4, 2018 •

edited

Loading

patrickhulce May 4, 2018 •

edited

Loading

hwikyounglee May 4, 2018

patrickhulce left a comment

patrickhulce May 4, 2018

patrickhulce May 4, 2018

paulirish May 4, 2018

patrickhulce May 4, 2018

paulirish commented May 4, 2018

paulirish commented May 4, 2018

patrickhulce left a comment

patrickhulce May 4, 2018

paulirish May 4, 2018

patrickhulce May 4, 2018

paulirish May 4, 2018

patrickhulce May 4, 2018

paulirish May 4, 2018

hwikyounglee May 4, 2018

report: new audit list display, indexes & new icons #5109

report: new audit list display, indexes & new icons #5109

Conversation

paulirish commented May 4, 2018 • edited Loading

example reports:

patrickhulce commented May 4, 2018 • edited Loading

vinamratasingal-zz commented May 4, 2018 • edited Loading

patrickhulce May 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish commented May 4, 2018

paulirish commented May 4, 2018

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish commented May 4, 2018 •

edited

Loading

patrickhulce commented May 4, 2018 •

edited

Loading

vinamratasingal-zz commented May 4, 2018 •

edited

Loading

patrickhulce May 4, 2018 •

edited

Loading