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: add explodey gauge for performance category #15396

Merged
merged 10 commits into from
Oct 2, 2023
Merged

Conversation

connorjclark
Copy link
Collaborator

@connorjclark connorjclark commented Aug 22, 2023

Adds the scoring calculator's neat gauge to navigation reports.

Took from https://github.com/paulirish/lh-scorecalc/tree/restore-unexpanded, but with some changes:

  • lh-gauge to lh-exp-gauge, to not conflict with LH's normal gauge
  • use metric.acronym instead of .id (there is a TODO for this in the calc codebase)
  • removed .toFixed and - 2*endDiffInner thing in arc calculation which made score of 100 have a gap. idk what this was for
  • add support for dark theme

demo: https://lighthouse-git-exploding-gauge-googlechrome.vercel.app/sample-reports/english/

@connorjclark
Copy link
Collaborator Author

connorjclark commented Aug 23, 2023

Screenshot 2023-08-23 at 11 09 08 AM

Should the passive state just remain like this (the height of the peek thing) instead of peeking / hiding?

Should clicking open the calculator?

@adamraine
Copy link
Member

Personal opinions and nits:

Should the passive state just remain like this (the height of the peek thing) instead of peeking / hiding?

I would say no, I think it looks kinda weird. Additionally, not sure if it's intended behavior but I do like the the gauge remains "exploded" after moving the mouse away (passive state changes after interacting once)

Should clicking open the calculator?

I'm leaning yes, but we shouldn't remove the other calculator link.

Some other thoughts:

  • I don't like the "?" cursor. It seems to imply there should be a tooltip and it's hard to tell what the cursor is pointing at. I would prefer just a normal cursor.
  • The size of the number in relation to the size of the entire gauge is different than it was before. I would prefer if the text size didn't change but this is a nit.

@brendankenny
Copy link
Member

  • Maybe it's my oldish macbook pro, but I feel like there's always been frame drops when I hover the gauge here and in the score calculator, and similar (but inconsistently) when hovering over the individual metric arcs. Any way to ameliorate this?
  • The colors make less sense outside of the context given by the score calculator's per metric coloring. Like in the example report, my first thought is why is TBT amber (needs improvement?) when it's so low and the metric is red in the main report. Similar for the blue CLS and red SI, etc. Should the colors all just switch to the scoring color since the point is to give a sense of how you did in each metric maps to the overall score? Less colorful but possibly easier to interpret.

Copy link
Member

@adamraine adamraine left a comment

Choose a reason for hiding this comment

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

I like the direction here, would be interested in landing alongside the new performance sorting

@@ -2067,8 +2060,6 @@ details[open] .lh-clump-toggletext--hide { display: block;}
stroke-width: 24;
/* hack. move the hover target out of the center. ideally i tweak the r instead but that rquires considerably more math. */
transform: scale(1.15);
/* I'll remove it when someone goes 'eh' */
Copy link
Member

Choose a reason for hiding this comment

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

I guess it was I who went 'eh'

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 am the one who ehs

@paulirish

@paulirish
Copy link
Member

but I do like the the gauge remains "exploded" after moving the mouse away

lol its actually not supposed to. that was a bug i wasnt able to squash. it's inconsistent and has a lot to do if you mouse out on the bottom vs the side.

I don't like the "?" cursor.

sgtm. im cool with dropping. (connor already dropped)

maybe it's my oldish macbook pro, but I feel like there's always been frame drops

Yeah I get this too. I made it a lot better than it was originally but there's still some layer mgmt improvements possible.

The colors make less sense outside

fair. (connor changed it)

@paulirish
Copy link
Member

other nits:

the main score number probably shouldnt change color to match each metric on hover. it fades out pretty quick, but its distracting that it goes from gray to orange/red before fading out. let's keep as grey while exploded.

check light mode on https://lighthouse-git-exploding-gauge-googlechrome.vercel.app/sample-reports/english/ looks like 'color-average-secondary' is still red in that case.. not orange.

in dark mode the lh-exp-gauge--faded arcs are too dim. can't see the faint red on black. definitely needs like 0.3 opacity in dark. light mode seems fine with 0.1 though.

image image

@paulirish
Copy link
Member

paulirish commented Sep 28, 2023

fwiw the main score text still changes color ..

  1. https://lighthouse-git-exploding-gauge-googlechrome.vercel.app/sample-reports/english/
  2. dark mode
  3. hover on the gauge
  4. move cursor over the CLS section
  5. watch the 28 go orange right before fading out

lightmode it fades to average-secondary which is very reddish so you can hardly tell.

@connorjclark
Copy link
Collaborator Author

I removed the calc link because it didn't play well with mobile (can't get the thing to hover without a tap, which would open a new tab).

report/assets/styles.css Outdated Show resolved Hide resolved
report/assets/styles.css Outdated Show resolved Hide resolved
report/renderer/dom.js Outdated Show resolved Hide resolved
@adamraine

This comment was marked as resolved.

@connorjclark connorjclark merged commit 6a4c751 into main Oct 2, 2023
26 of 27 checks passed
@connorjclark connorjclark deleted the exploding-gauge branch October 2, 2023 20:30
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.

5 participants