Skip to content

Commit

Permalink
feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
paulirish committed May 14, 2018
1 parent d538df8 commit c1359fc
Show file tree
Hide file tree
Showing 4 changed files with 22 additions and 8 deletions.
15 changes: 12 additions & 3 deletions lighthouse-core/report/html/renderer/category-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,20 @@ class CategoryRenderer {
/**
* @param {!ReportRenderer.AuditJSON} audit
* @param {number} index
* @param {DocumentFragment=} providedTmpl
* @return {!Element}
*/
renderAudit(audit, index, providedTmpl) {
const tmpl = providedTmpl || this.dom.cloneTemplate('#tmpl-lh-audit', this.templateContext);
renderAudit(audit, index) {
const tmpl = this.dom.cloneTemplate('#tmpl-lh-audit', this.templateContext);
return this.populateAuditValues(audit, index, tmpl);
}

/**
* @param {!ReportRenderer.AuditJSON} audit
* @param {number} index
* @param {!DocumentFragment} tmpl
* @return {!Element}
*/
populateAuditValues(audit, index, tmpl) {
const auditEl = this.dom.find('.lh-audit', tmpl);
auditEl.id = audit.result.id;
const displayTextEl = this.dom.find('.lh-audit__display-text', auditEl);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ class PerformanceCategoryRenderer extends CategoryRenderer {
*/
_renderOpportunity(audit, index, scale) {
const oppTmpl = this.dom.cloneTemplate('#tmpl-lh-opportunity', this.templateContext);
const element = this.renderAudit(audit, index, oppTmpl);
const element = this.populateAuditValues(audit, index, oppTmpl);
element.classList.add(`lh-audit--${Util.calculateRating(audit.result.score)}`);
element.id = audit.result.id;

Expand Down
6 changes: 2 additions & 4 deletions lighthouse-core/report/html/renderer/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,18 +39,16 @@ class Util {
if (typeof displayValue === 'string') return displayValue;
if (!displayValue) return '';

// Don't mutate the value
displayValue = [...displayValue];
const replacementRegex = /%([0-9]*(\.[0-9]+)?d|s)/;
const template = /** @type {string} */ (displayValue.shift());
const template = /** @type {string} */ (displayValue[0]);
if (typeof template !== 'string') {
// First value should always be the format string, but we don't want to fail to build
// a report, return a placeholder.
return 'UNKNOWN';
}

let output = template;
for (const replacement of displayValue) {
for (const replacement of displayValue.slice(1)) {
if (!replacementRegex.test(output)) {
// eslint-disable-next-line no-console
console.warn('Too many replacements given');
Expand Down
7 changes: 7 additions & 0 deletions lighthouse-core/test/report/html/renderer/util-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -135,4 +135,11 @@ describe('util helpers', () => {
'Too many replacements given',
]);
});

it('does not mutate the provided array', () => {
const displayValue = ['one:%s, two:%s', 'foo', 'bar'];
const cloned = JSON.parse(JSON.stringify(displayValue));
Util.formatDisplayValue(displayValue);
assert.deepStrictEqual(displayValue, cloned, 'displayValue was mutated');
});
});

0 comments on commit c1359fc

Please sign in to comment.