core(i18n): export rendererFormattedStrings #5713
Conversation
| @@ -89,9 +89,10 @@ class CategoryRenderer { | |||
| // Add list of warnings or singular warning | |||
| const warningsEl = this.dom.createChildOf(titleEl, 'div', 'lh-warnings'); | |||
| if (warnings.length === 1) { | |||
| warningsEl.textContent = `Warning: ${warnings.join('')}`; | |||
| // TODO(i18n): Maybe need to construct this differently | |||
There was a problem hiding this comment.
TODO(i18n): Maybe need to construct this differently
if warnings are full sentences this should be fine
There was a problem hiding this comment.
alright, removed
| @@ -35,6 +35,11 @@ class ReportRenderer { | |||
| renderReport(report, container) { | |||
| // If any mutations happen to the report within the renderers, we want the original object untouched | |||
| const clone = /** @type {LH.ReportResult} */ (JSON.parse(JSON.stringify(report))); | |||
| // Mutate the UIStrings if necessary (while saving originals) | |||
There was a problem hiding this comment.
should the technical debt comment below be moved up here? :)
It does kind of suck to rely on utils as a mutable singleton. We pass at least a DOM into all of our report rendering classes. Should we pass in strings? Or pass the lhr to everything, use lhr.i18n.rendererFormattedStrings.string everywhere, and here (since we're mutating the lhr anyways) could set lhr.i18n.rendererFormattedStrings to the default strings if not set.
Fine to punt to a future PR, but it would be nice to have a plan before we get too deep.
There was a problem hiding this comment.
Or pass the lhr to everything, use lhr.i18n.rendererFormattedStrings.string everywhere, and here (since we're mutating the lhr anyways) could set lhr.i18n.rendererFormattedStrings to the default strings if not set.
This sounds good to me, we have other LHR reaching needs like locale in config settings and such that would be nice. I accept your offer for a followup :)
| @@ -35,6 +35,11 @@ class ReportRenderer { | |||
| renderReport(report, container) { | |||
| // If any mutations happen to the report within the renderers, we want the original object untouched | |||
| const clone = /** @type {LH.ReportResult} */ (JSON.parse(JSON.stringify(report))); | |||
| // Mutate the UIStrings if necessary (while saving originals) | |||
| ReportRenderer.stashUIStrings(); | |||
There was a problem hiding this comment.
if it's stashing just for this function, can it stored in a local variable instead of stashed in a static property?
brings i18n strings to the renderer! this tackles most of what we need for performance category. strategy is what was outlined in #5655, strings are located at
lhr.i18n.rendererFormattedStringswhich the renderer uses to replace its own UIStrings inutil.jsthe
${n} auditsis definitely going to be a problem we're going to have to figure out, but for performance it only shows up for the "passed audits" section and none of the others which isn't the worst thing to live without at first if we need to