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: use acronyms and round metrics for shorter calc url #10954

Merged
merged 4 commits into from
Jun 19, 2020

Conversation

connorjclark
Copy link
Collaborator

@connorjclark connorjclark requested a review from a team as a code owner June 12, 2020 22:44
@connorjclark connorjclark requested review from Beytoven and removed request for a team June 12, 2020 22:44
* @param {number} val
* @return {number}
*/
const clampTo2Decimals = val => Math.round(val * 100) / 100;
Copy link
Collaborator

Choose a reason for hiding this comment

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

feels like we should just completely round everything but CLS

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok. wonder what the chances are of this rounding resulting in an off-by-1 perf score :p

/nerdsnipe @brendankenny

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

lol @brendankenny is this what kicked off #13316

@@ -116,10 +116,29 @@ class PerformanceCategoryRenderer extends CategoryRenderer {
if (fci) v5andv6metrics.push(fci);
if (fmp) v5andv6metrics.push(fmp);

/** @type {Record<string, string>} */
const acronymMapping = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this seems like it could be useful a lot of place haha

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would have added it to the audit meta but that had TS implications I didn't want to address.

Copy link
Member

Choose a reason for hiding this comment

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

@@ -116,10 +116,29 @@ class PerformanceCategoryRenderer extends CategoryRenderer {
if (fci) v5andv6metrics.push(fci);
if (fmp) v5andv6metrics.push(fmp);

/** @type {Record<string, string>} */
const acronymMapping = {
Copy link
Member

Choose a reason for hiding this comment

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

@paulirish
Copy link
Member

really wish this was like google3 where i can say LGTM but simultaneously leave "unresolved" comments.

@connorjclark
Copy link
Collaborator Author

connorjclark commented Jun 17, 2020

lol What (flake).

image

@patrickhulce
Copy link
Collaborator

lol What (flake).

CI is reallllllyyyy slow what'd I tell ya :)

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.

6 participants