Conversation
paulirish
left a comment
There was a problem hiding this comment.
looking at your screenshot i dont know why you dont see the outerhtml snippet. this is what i had on paulirish.com:
| * @return {LH.Audit.Details.Table['items']} | ||
| */ | ||
| static getNodeData(traceNodes) { | ||
| const lcpNode = traceNodes.find(node => node.type === 'lcp'); |
There was a problem hiding this comment.
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.
Opting for metricTag as a replacement
There was a problem hiding this comment.
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.
could you take care of these lint issues?
| * @param {string} attributeName | ||
| * @param {string} attributeValue | ||
| */ | ||
| async setNodeAttribute(nodeId, attributeName, attributeValue) { |
There was a problem hiding this comment.
IMO its fine to move this method back into your trace-nodes gatherer. and just inline it.
| 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.
convention would be adding something like // HACK! to call out this is a temporary thing ;)
| 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 */ |
|
@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.
| metricTag: string; | ||
| selector: string; | ||
| nodeLabel?: string; | ||
| nodePath: string; |
There was a problem hiding this comment.
devtoolsNodePath or path is what we use in the artifacts. so let's go with devtoolsNodePath
There was a problem hiding this comment.
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.
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.
connorjclark
left a comment
There was a problem hiding this comment.
just some nits but LGTM!
Co-authored-by: Connor Clark <cjamcl@google.com>
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.