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: redesign runtime settings #13125

Merged
merged 37 commits into from
Oct 21, 2021
Merged

report: redesign runtime settings #13125

merged 37 commits into from
Oct 21, 2021

Conversation

paulirish
Copy link
Member

fixes #10962

"design doc": https://docs.google.com/document/d/1W66PKNDdcyNYDHnDBPvplyLAUfozdgk9FAZ3pjGieKA/edit#

mock, though I've evolved it after working with things for a bit:
image

screenshot:

image

Admittedly, this takes some shortcuts with i18n.. but also the runtime settings never got full i18n treatment. (That said, I'd like to rectify that :)

@paulirish paulirish requested a review from a team as a code owner September 25, 2021 23:15
@paulirish paulirish requested review from adamraine and removed request for a team September 25, 2021 23:15
@google-cla google-cla bot added the cla: yes label Sep 25, 2021
Comment on lines 114 to 118
'devices',
`Lighthouse ${report.lighthouseVersion}, ${envValues.deviceEmulation}`,
`Host CPU Power: ${report?.environment.benchmarkIndex.toFixed(0)}. ` +
`CPU throttling: ${envValues.cpuThrottling}. ` +
`${Util.i18n.strings.runtimeSettingsAxeVersion}: ${axeVersion}`,
Copy link
Member

Choose a reason for hiding this comment

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

WDYT about splitting this into three icons:

  • Device emulation: Would use this "devices" icon
  • CPU throttling: CPU icon (see image)
  • Lighthouse version + axe version: Could use a grayscale lighthouse logo

I want to make sure our icon usage is consistent across reports. In the flow report we are currently using the "devices" icon exclusively for emulation settings:

Screen Shot 2021-09-27 at 3 51 41 PM

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks for flagging this. i hadn't spotted those.

LH icon.. this visual language arose out of a need to differentiate lab/field.. but it wouldnt make sense to have an LH entry in the CrUX one.

cpu + device sharing an entry.. yeah i can somewhat defend this as we're trying to minimize the size of this thing. but.. breaking out CPU Throttling (and benchmark index) would make sense in flow where you have plenty of space.

the other bits i brought up in comments https://docs.google.com/document/d/1W66PKNDdcyNYDHnDBPvplyLAUfozdgk9FAZ3pjGieKA/edit# .. we can iterate there.

@connorjclark connorjclark changed the title report: redesign runtime settings UI as env meta block report: redesign runtime settings Sep 29, 2021
/** Descriptive label that this analysis run was from a single pageload of a browser (not a summary of hundreds of loads) */
runtimeSingleLoad: 'Single page load',
/** Descriptive explanation that this analysis run was from a single pageload of a browser, whereas field data often summarizes hundreds+ of page loads */
runtimeSingleLoadTooltip: 'This data represents a single page load. Field data often summarizes many sessions.', // eslint-disable-line max-len
Copy link
Member

Choose a reason for hiding this comment

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

Slight reword, to make it clear that this is not field data.

Suggested change
runtimeSingleLoadTooltip: 'This data represents a single page load. Field data often summarizes many sessions.', // eslint-disable-line max-len
runtimeSingleLoadTooltip: 'This data is taken from a single page load, as opposed to field data summarizing many sessions.', // eslint-disable-line max-len

report/renderer/report-renderer.js Outdated Show resolved Hide resolved
break;
case 'devtools': {
const {cpuSlowdownMultiplier, requestLatencyMs} = throttling;
cpuThrottling = `${Util.i18n.formatNumber(cpuSlowdownMultiplier)}x slowdown (DevTools)`;
networkThrottling = `${Util.i18n.formatNumber(requestLatencyMs)}${NBSP}ms HTTP RTT, ` +
`${Util.i18n.formatNumber(throttling.downloadThroughputKbps)}${NBSP}Kbps down, ` +
`${Util.i18n.formatNumber(throttling.uploadThroughputKbps)}${NBSP}Kbps up (DevTools)`;

const isSlow4G = requestLatencyMs === 150 * 3.75;
Copy link
Member

Choose a reason for hiding this comment

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

Could we use a isSlow4G function here that checks every throttling value, not just rtt?

Copy link
Member Author

Choose a reason for hiding this comment

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

sure. done!

report/assets/styles.css Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update runtime settings UI, add to PSI
4 participants