Conversation
brendankenny
left a comment
There was a problem hiding this comment.
Globals weren't being provided automatically
are these loading in devtools as separate files? It's weird that they can't find each other
| */ | ||
| constructor(document) { | ||
| this._dom = new DOM(document); | ||
| constructor(document, DOM, DetailsRenderer) { |
There was a problem hiding this comment.
did you mean to delete this._dom construction?
| */ | ||
| constructor(document) { | ||
| this._dom = new DOM(document); | ||
| constructor(document, DOM, DetailsRenderer) { |
There was a problem hiding this comment.
also, should they be passed the constructor or an instance?
There was a problem hiding this comment.
probably need to update the tests too
There was a problem hiding this comment.
also, should they be passed the constructor or an instance
yeah, I'm in favor of passing in already-constructed instances
| if (typeof module !== 'undefined' && module.exports) { | ||
| module.exports = DOM; | ||
| } else if (self) { | ||
| self.DOM = DOM; |
There was a problem hiding this comment.
typeof self !== 'undefined' for all of these? I'm ambivalent between that and just having else { } since I can't come up with a case where module and self are both undefined :)
|
ptal! |
| /** | ||
| * @param {!Document} document | ||
| * @param {!DOM} DOM | ||
| * @param {!DetailsRenderer} DetailsRenderer |
| <script> | ||
| const renderer = new ReportRenderer(document); | ||
| document.body.appendChild(renderer.renderReport(window.__LIGHTHOUSE_JSON__)); | ||
| const dom = new DOM(document); |
There was a problem hiding this comment.
Ha, DI! I feel like we'll have to move more implementation out as time goes on. I guess it's fine for now.
| @@ -48,7 +49,7 @@ class DOM { | |||
| * @throws {Error} | |||
| */ | |||
| cloneTemplate(selector) { | |||
There was a problem hiding this comment.
For #2000, I think this should be:
cloneTemplate(selector, context = this.templatesContext) {
const template = context.querySelector(selector);
...
}
so we can override where to look for subtemplates.
|
okay @brendankenny @ebidel. all ready for ya? |
brendankenny
left a comment
There was a problem hiding this comment.
LGTM. Just some nits
| * helpText: string, | ||
| * score: (number|boolean), | ||
| * scoringMode: string, | ||
| * details: (DetailsRenderer.DetailsJSON|DetailsRenderer.CardsDetailsJSON|undefined) |
There was a problem hiding this comment.
(!DetailsRenderer.DetailsJSON|!DetailsRenderer.CardsDetailsJSON|undefined) I think
| * weight: number, | ||
| * score: number, | ||
| * description: string, | ||
| * audits: Array<ReportRenderer.AuditJSON> |
There was a problem hiding this comment.
!Array<!ReportRenderer.AuditJSON>
| } | ||
|
|
||
| /** | ||
| * @param {!Document|!Element} context |
There was a problem hiding this comment.
maybe a function description for why you'd want to inject this? there won't be any hints on the LH side
| */ | ||
| createElement(name, className, attrs = {}) { | ||
| createElement(name, className, attrs) { | ||
| attrs = attrs || {}; |
There was a problem hiding this comment.
TODO switch this back when https://codereview.chromium.org/2821773002/ lands?
| * @typedef {{ | ||
| * type: string, | ||
| * text: (string|undefined), | ||
| * header: (DetailsRenderer.DetailsJSON|undefined), |
There was a problem hiding this comment.
(!DetailsRenderer.DetailsJSON|undefined)
| /** @typedef {{ | ||
| * type: string, | ||
| * text: string, | ||
| * header: DetailsJSON, |
| * type: string, | ||
| * text: string, | ||
| * header: DetailsJSON, | ||
| * items: Array<{title: string, value: string, snippet: string|undefined, target: string}> |
There was a problem hiding this comment.
!Array. did it need parens around snippet's type?
Gettin the new report going in DevTools and wanted some changes like these.
e.g. my subclass currently looks like (UPDATED):
Screenshot btw:
