-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
core: surface LCP element in a diagnostic audit #10517
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.
* @return {LH.Audit.Details.Table['items']} | ||
*/ | ||
static getNodeData(traceNodes) { | ||
const lcpNode = traceNodes.find(node => node.type === 'lcp'); |
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.
seeing this type
so close to the type
thats on L59 makes me think this one should have a different name.. we could go all the way to auditRef
or stay with a fairly loose tag
.
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.
Opting for metricTag
as a replacement
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.
Not sure where tag
came from, but I don't like it. It's being used as an id
, but we could just be specific and say metric
, since we only plan to have trace nodes that associate with exactly one metric. If that does not hold true sometime in the future, that's fine and we can just change it to id
with no issue since this is just an artifact.
*/ | ||
function collectTraceNodes() { | ||
/** @type {Array<HTMLElement>} */ | ||
// @ts-ignore - put into scope via stringification |
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.
could you take care of these lint issues?
lighthouse-core/gather/driver.js
Outdated
* @param {string} attributeName | ||
* @param {string} attributeValue | ||
*/ | ||
async setNodeAttribute(nodeId, attributeName, attributeValue) { |
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.
IMO its fine to move this method back into your trace-nodes gatherer. and just inline it.
@@ -41,6 +41,11 @@ class PerformanceCategoryRenderer extends CategoryRenderer { | |||
const descriptionEl = this.dom.find('.lh-metric__description', tmpl); | |||
descriptionEl.appendChild(this.dom.convertMarkdownLinkSnippets(audit.result.description)); | |||
|
|||
if (audit.result.details) { |
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.
convention would be adding something like // HACK!
to call out this is a temporary thing ;)
@@ -1522,3 +1522,5 @@ | |||
75% { opacity: 1; } | |||
100% { opacity: 1; filter: drop-shadow(1px 0px 1px #aaa) drop-shadow(0px 2px 4px hsla(206, 6%, 25%, 0.15)); pointer-events: auto; } | |||
} | |||
|
|||
/*# sourceURL=report-styles.css */ |
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.
👍
@Beytoven could you also add the CLS nodes into this PR as well? |
I don't love hiding this under the metric description toggle. What are some alternatives? At minimum, we could auto-expand metrics if LCP is bad (but would like to see another approach) |
Neither do I. Adding it there was just easiest for seeing how it shows up in the report. CLS will look even worse since the description is already pretty long. Having a separate, collapsable section may work better as we discussed last week. |
FWIW this fits the bill as a Diagnostic, so it has a collapsable place already (and could eventually be an opportunity since we have the info on e.g. earlier LCP events). I don't know if that's a good idea until the UI changes tying opportunities/diagnostics to particular metrics make the connection clearer, and I don't know if anyone knows a timeline for that :) |
This reverts commit 597c071.
types/artifacts.d.ts
Outdated
metricTag: string; | ||
selector: string; | ||
nodeLabel?: string; | ||
nodePath: string; |
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.
devtoolsNodePath
or path
is what we use in the artifacts. so let's go with devtoolsNodePath
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 rest of these names have consistent names in the artifacts and we're all good.
|
||
const UIStrings = { | ||
/** Descriptive title of a diagnostic audit that provides the element that was determined to be the Largest Contentful Paint. */ | ||
title: 'Largest Contentful Paint element', |
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.
yeah calling this traceElements works for me. lets change to that.
lol even getNodeSelector
and getNodeLabel
are documented as taking Element/HTMLElement and will fail if passed a textnode/comment node.
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.
just some nits but LGTM!
Co-authored-by: Connor Clark <[email protected]>
Surfaces the element that is flagged as the LargestContentfulPaint in the LH report. Giving users the specific element gives them a good starting point when looking to reduce their LCP time.