report(redesign): score icons, audit group headers, colors #8222
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -54,22 +54,20 @@ class CategoryRenderer { | |
|
|
||
| /** | ||
| * @param {LH.ReportResult.AuditRef} audit | ||
| * @return {Element} | ||
| */ | ||
| renderAudit(audit) { | ||
| const tmpl = this.dom.cloneTemplate('#tmpl-lh-audit', this.templateContext); | ||
| return this.populateAuditValues(audit, tmpl); | ||
| } | ||
|
|
||
| /** | ||
| * Populate an DOM tree with audit details. Used by renderAudit and renderOpportunity | ||
| * @param {LH.ReportResult.AuditRef} audit | ||
| * @param {DocumentFragment} tmpl | ||
| * @return {Element} | ||
| */ | ||
| populateAuditValues(audit, tmpl) { | ||
| const auditEl = this.dom.find('.lh-audit', tmpl); | ||
| auditEl.id = audit.result.id; | ||
| const scoreDisplayMode = audit.result.scoreDisplayMode; | ||
|
|
@@ -109,7 +107,6 @@ class CategoryRenderer { | |
| header.appendChild(elem); | ||
| } | ||
| } | ||
|
|
||
| // Add chevron SVG to the end of the summary | ||
| this.dom.find('.lh-chevron-container', auditEl).appendChild(this._createChevron()); | ||
|
|
@@ -193,16 +190,18 @@ class CategoryRenderer { | |
| */ | ||
| renderAuditGroup(group) { | ||
| const groupEl = this.dom.createElement('div', 'lh-audit-group'); | ||
|
|
||
| // Group header DOM roughly matches the clump header DOM, and that's OK. | ||
|
There was a problem hiding this comment. should remove this comment. See reasoning in earlier thread, but, more importantly, this wouldn't be the place to put the comment anyways :) This group header is using |
||
| const auditGroupHeader = this.dom.createElement('div', 'lh-audit-group__header'); | ||
|
There was a problem hiding this comment. @brendankenny over in templates i think you left right here we're recreating a bit of dom which is nearly the same as the DOM we see in the clump template. I suppose that's OK at this point, but i just wanted to call it out. (These days I dont think we have any groups within non-passed clumps anymore so its a bit different) In this case I think i'd love it if we just leave a comment like There was a problem hiding this comment.
done i may try combining these two into the sample template later, in the clean up phase. There was a problem hiding this comment.
I was serious about the original TODO :)
They don't need to be the same thing, and we don't win anything by creating a grand unified clump-group theorem except a new headache for every point where they're almost but not quite the same thing :P Seriously, we'll save like two total lines of code and gain some extra conditionals and a bunch of head scratching later about why groups are sometimes what we call groups everywhere else and why they're sometimes something completely different. Rather than make a comment here, the clean up phase should just |
||
|
|
||
| this.dom.createChildOf(auditGroupHeader, 'span', 'lh-audit-group__title') | ||
| .textContent = group.title; | ||
| if (group.description) { | ||
| const descriptionEl = this.dom.convertMarkdownLinkSnippets(group.description); | ||
| descriptionEl.classList.add('lh-audit-group__description'); | ||
| auditGroupHeader.appendChild(descriptionEl); | ||
| } | ||
| groupEl.appendChild(auditGroupHeader); | ||
|
|
||
| return groupEl; | ||
| } | ||
|
|
@@ -232,14 +231,12 @@ class CategoryRenderer { | |
|
|
||
| /** @type {Array<Element>} */ | ||
| const auditElements = []; | ||
|
|
||
| for (const [groupId, groupAuditRefs] of grouped) { | ||
| if (groupId === notAGroup) { | ||
| // Push not-grouped audits individually. | ||
| for (const auditRef of groupAuditRefs) { | ||
| auditElements.push(this.renderAudit(auditRef)); | ||
| } | ||
| continue; | ||
| } | ||
|
|
@@ -248,7 +245,7 @@ class CategoryRenderer { | |
| const groupDef = groupDefinitions[groupId]; | ||
| const auditGroupElem = this.renderAuditGroup(groupDef); | ||
| for (const auditRef of groupAuditRefs) { | ||
| auditGroupElem.appendChild(this.renderAudit(auditRef)); | ||
| } | ||
| auditGroupElem.classList.add(`lh-audit-group--${groupId}`); | ||
| auditElements.push(auditGroupElem); | ||
|
|
@@ -292,17 +289,15 @@ class CategoryRenderer { | |
|
|
||
| const headerEl = this.dom.find('.lh-audit-group__header', clumpElement); | ||
| const title = this._clumpTitles[clumpId]; | ||
| this.dom.find('.lh-audit-group__title', headerEl).textContent = title; | ||
| if (description) { | ||
| const descriptionEl = this.dom.convertMarkdownLinkSnippets(description); | ||
| descriptionEl.classList.add('lh-audit-group__description'); | ||
| headerEl.appendChild(descriptionEl); | ||
| } | ||
|
|
||
| const itemCountEl = this.dom.find('.lh-audit-group__itemcount', clumpElement); | ||
| itemCountEl.textContent = `(${auditRefs.length})`; | ||
|
There was a problem hiding this comment. @exterkamp help There was a problem hiding this comment. This one looks A-Okay because the final output is all numeric, correct? The concern here is non latin numerics, like ar: But afaik we already don't support that yet in LH or PSI consistently so I'd leave in the i18n TODO, but LGTM for now. |
||
|
|
||
| // Add all audit results to the clump. | ||
| const auditElements = auditRefs.map(this.renderAudit.bind(this)); | ||
|
|
||
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.
Yay no more numbering!