-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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: implement new design for opportunities #5115
Conversation
few UI notes:
|
this.dom.find('.lh-load-opportunity__sparkline', tmpl).title = displayValue; | ||
this.dom.find('.lh-load-opportunity__wasted-stat', tmpl).title = displayValue; | ||
this.dom.find('.lh-sparkline__bar', tmpl).style.width = summaryInfo.wastedMs / scale * 100 + '%'; | ||
this.dom.find('.lh-load-opportunity__wasted-stat', tmpl).textContent = Util.formatSeconds(summaryInfo.wastedMs); |
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.
we should probably filter on wastedMs < 50
now that it could show 0 s
@@ -116,6 +116,16 @@ class Util { | |||
return `${coarseTime.toLocaleString()}${NBSP}ms`; | |||
} | |||
|
|||
/** | |||
* @param {number} ms | |||
* @param {number=} granularity Controls how coarse the displayed value is, defaults to 10 |
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.
defaults to 0.1
if (!summaryInfo || !summaryInfo.wastedMs) { | ||
return element; | ||
} | ||
|
||
const elemAttrs = {title: Util.formatDisplayValue(audit.result.displayValue)}; | ||
const sparklineContainerEl = this.dom.createChildOf(summary, 'div', |
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.
yay cleanup! 🎉
color: var(--medium-75-gray); | ||
text-align: center; | ||
display: unset; | ||
line-height: 40px; |
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.
is this some variable +
/*
some padding?
margin: 0 5px; | ||
} | ||
.lh-load-opportunity__col--one { | ||
flex: 5; |
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.
I must say this was slightly magical to me given the header and details have different content, I have much to learn 📖 🤓
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
As the new UI settles it would be good to start asserting some of this stuff in the viewer/extension test. They're so barebones it will be a lot easier to add the tests as we go rather than at the end :)
…images'" This reverts commit c22e047.
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!
screenshots
example reports:
https://lonely-lumber.surge.sh/cnn.html
https://lonely-lumber.surge.sh/tinyhouse.html
https://lonely-lumber.surge.sh/dbwtester.html
https://lonely-lumber.surge.sh/example.html
https://lonely-lumber.surge.sh/paulirish.html
https://lonely-lumber.surge.sh/cnn.devtools.html
https://lonely-lumber.surge.sh/tinyhouse.devtools.html
https://lonely-lumber.surge.sh/dbwtester.devtools.html
https://lonely-lumber.surge.sh/example.devtools.html
https://lonely-lumber.surge.sh/paulirish.devtools.html
ref #5008