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: adjust score's arc length accounting for rounded linecap #9913

Merged
merged 9 commits into from
Dec 12, 2019

Conversation

paulirish
Copy link
Member

@paulirish paulirish commented Nov 3, 2019

I noticed that with the rounded linecap, the arc visually extends farther than it really should.

image

So here's the proper adjustment. After this change, the arc shortens up a little bit on both ends.

partial fix of #9803


UPDATE. I tweaked the math.. the arcs now go halfway between the two cases above.
image

GIF of before and after (updated):
Kapture 2019-11-04 at 11 59 19

@@ -1134,7 +1133,6 @@
text-decoration: none;
padding: var(--score-container-padding);

--circle-border-width: 8;
Copy link
Member Author

Choose a reason for hiding this comment

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

In order to keep this circle-stroke-width var in CSS, we'd need to use both requestAnimationFrame and getComputedStyle in order to read it. That seemed not worth it.

(And currently we never change this width)

Copy link
Collaborator

Choose a reason for hiding this comment

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

seems like it should be relative to the viewbox specified in the svg anyhow so seems even better 👍

@connorjclark
Copy link
Collaborator

connorjclark commented Nov 3, 2019

wanna take care of #9803 too? (the part where 0 should not show an arc)

@connorjclark
Copy link
Collaborator

the 100 is off now :(

@paulirish
Copy link
Member Author

the 100 is off now :(

Yeah...... I don't love the 100 now. It's gotta be a perfect ring.

@patrickhulce
Copy link
Collaborator

If the end of the line weren't rounded I would agree this is the appropriate fix, but given that this change makes only the tip extend to where the line should be, it visually feels like the line does not extend far enough now because we have a grand total of like 2 pixels on the actual line.. 100 makes this obvious but a 25 too feels like it's just short of a quarter instead of being an actual quarter circle. WDYT about compensating for this fact and splitting the difference between master and this PR?

@paulirish
Copy link
Member Author

paulirish commented Nov 3, 2019

WDYT about compensating for this fact and splitting the difference between master and this PR?

Yeah I think that's the solution.

I think there's four total options:

  • Trim back the arc on both ends (current impl - shown above)
  • Trim back the arc on one end and fix the 0/start alignment
  • Trim back the arc on one end and fix the end alignment
  • Half-trim back the arc on both ends

@yuinchien what do you think?

Also this marks the first time that trigonometry has assisted in my career. Thanks Mr Casarella!

@yuinchien
Copy link
Collaborator

Love it! I think for 0, we don't need the tiny circle. 100 should be full circle yes. Looks great, thanks for the detail attention to visual!

@paulirish
Copy link
Member Author

updated. i implemented the "Half-trim back the arc on both ends" option.

and:

  • 0 has no dot
  • 1 and 2 are differentiable
  • 100 is a full ring
  • 99 has a satisfying amount of dimple

gif at the top is updated.

@brendankenny
Copy link
Member

gif at the top is updated.

lies?

@paulirish
Copy link
Member Author

paulirish commented Nov 4, 2019

gif at the top is updated.

lies?

oops didn't hit submit. thx done.
update: i updated the gif yet again.

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

some optional nits, but otherwise new gauges LGTM! 🎛️

/**
* Define the score arc of the gauge
* Credit to xgad for the original technique: https://codepen.io/xgad/post/svg-radial-progress-meters
* @param {SVGCircleElement} elem
Copy link
Member

Choose a reason for hiding this comment

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

arcElem?

* @param {SVGCircleElement} elem
* @param {number} percent
*/
_setGaugeArc(elem, percent) {

This comment was marked as resolved.

This comment was marked as resolved.

lighthouse-core/report/html/renderer/category-renderer.js Outdated Show resolved Hide resolved
lighthouse-core/report/html/renderer/category-renderer.js Outdated Show resolved Hide resolved
// The rounded linecap of the stroke extends the arc past its start & end.
// First, we tweak the -90deg rotation to adjust
const strokeWidthPx = Number(elem.getAttribute('stroke-width'));
const rotationalAdjustmentPercent = 0.25 * strokeWidthPx / circumferencePx;
Copy link
Member

Choose a reason for hiding this comment

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

maybe pull the 0.25 (and the /2 below) into a constant for easy tweaking in the future?

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

LGTM! the best of all the worlds! 😍

lighthouse-core/report/html/renderer/category-renderer.js Outdated Show resolved Hide resolved
@@ -1134,7 +1133,6 @@
text-decoration: none;
padding: var(--score-container-padding);

--circle-border-width: 8;
Copy link
Collaborator

Choose a reason for hiding this comment

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

seems like it should be relative to the viewbox specified in the svg anyhow so seems even better 👍

Co-Authored-By: Brendan Kenny <bckenny@gmail.com>
Co-Authored-By: Brendan Kenny <bckenny@gmail.com>
@vercel vercel bot temporarily deployed to staging November 10, 2019 06:12 Inactive
Co-Authored-By: Patrick Hulce <patrick.hulce@gmail.com>
@paulirish paulirish merged commit a7133e5 into master Dec 12, 2019
@paulirish paulirish deleted the gaugearc branch December 12, 2019 00:32
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.

None yet

6 participants