-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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: move runtime settings to footer #5295
Conversation
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 like it in the footer, feels much cleaner!
this._dom.find('.lh-env__description', item).textContent = runtime.description; | ||
env.appendChild(item); | ||
}); | ||
url.href = url.textContent = toolbarUrl.href = toolbarUrl.textContent = report.finalUrl; |
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.
😮
position: absolute; | ||
width: 100%; | ||
background-color: var(--header-bg-color); | ||
font-size: 12px; |
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.
the new text feels a tad big, wdyt about keeping 12px?
EDIT: well I guess the font-size-adjust thing explains this
} | ||
span.lh-env__description { | ||
font-family: var(--monospace-font-family); | ||
font-size-adjust: 0.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.
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.
yup whoops. i had on experimental web platform features. i'll do this without FSA.
One broader question: is there anyway we can have just the device and network settings in the header? (or a summary of it). I feel like a question we often get is "What device/network configuration is this Lighthouse run reflective of" Having that front and center would be nice :) |
aye, though this will rarely be changing. so people just need to see it like once. IMO at the bottom / visible by default is already a lot more visible than previously, so I think this audience is probably satisfied by the current PR.
It's a lot of text, so it'd have to be pretty minimal. Hwi had something along these lines before. It works for me. |
okay summary line added to the report header, it can be these strings:
also it's a subtle link and can be clicked to see all the details in the bottom. tweaked styles in the bottom |
Small nit: Can the summary say "3G network" Otherwise, LGTM |
|
||
const emuDescs = Util.getEmulationDescriptions(report.configSettings || {}); |
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.
the lation
is just too much, huh?
let's use emulation and nix the /** @type HTMLAnchorElement
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
break; | ||
case 'devtools': { | ||
const {cpuSlowdownMultiplier, requestLatencyMs} = throttling; | ||
cpuThrottling = `${Util.formatNumber(cpuSlowdownMultiplier)}x slowdown (DevTools)`; | ||
networkThrottling = `${Util.formatNumber(requestLatencyMs)}${NBSP}ms HTTP RTT, ` + | ||
`${Util.formatNumber(throttling.downloadThroughputKbps)}${NBSP}Kbps down, ` + | ||
`${Util.formatNumber(throttling.uploadThroughputKbps)}${NBSP}Kbps up (DevTools)`; | ||
summary = 'Throttled Fast–3G network'; |
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 don't think we style this as Fast-3G
anywhere else, just Fast 3G
?
tests need some work though it seems |
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.
LGTM3!
screenshots
deployment should have it, too.