Report v2: port over cards formatter#1992
Conversation
| * @param {!DetailsJSON} list | ||
| * @return {!Element} | ||
| */ | ||
| _renderCards(list) { |
| _renderCards(list) { | ||
| const element = this._dom.createElement('details', 'lighthouse-details'); | ||
| if (list.header) { | ||
| element.appendChild(this._dom.createElement('summary')).textContent = list.header.text; |
There was a problem hiding this comment.
why are we keeping the verbose header: {type: 'text', text: 'View details'} format if we don't do anything with it (here or in the list formatter)?
There was a problem hiding this comment.
You mean just use a string: header: 'View details'?
There was a problem hiding this comment.
You mean just use a string:
header: 'View details'?
or whatever. I'm just wondering why it's a full DetailsJSON if we don't use it
| element.appendChild(this._dom.createElement('summary')).textContent = list.header.text; | ||
| } | ||
|
|
||
| const items = this._dom.createElement('div', 'lighthouse-scorecards'); |
There was a problem hiding this comment.
nit: need to differentiate types of things here. maybe call this parentElement or something?
| } | ||
|
|
||
| /** | ||
| * @param {!DetailsJSON} list |
There was a problem hiding this comment.
this isn't a DetailsJSON, though (specifically list.items isn't a Array<DetailsJSON>|undefined). Maybe you want to create a new CardsDetailsJSON or whatever? I'm also not clear where this leaves us on the whole "new details property has a standard structure" thing
| } | ||
| Object.keys(attrs).forEach(key => { | ||
| element.setAttribute(key, attrs[key]); | ||
| if (attrs[key] !== undefined) { |
There was a problem hiding this comment.
need to update the attrs type signature if adding this, but it would probably be better to keep it strict and make the caller correct it if they don't want an attribute
There was a problem hiding this comment.
{!Object<?string, ?string>=}?
There was a problem hiding this comment.
if it shows up in Object.keys() but is undefined then it would be {!Object<string, (string|undefined)>=} but it's still silly to be passing in undefined attributes. If it's just in that one spot, why not make a const attributes = item.snippet ? {title: item.snippet} : undefined or whatever there
There was a problem hiding this comment.
Every caller will have to do that. I'd rather just prevent attr="undefined" in one place. Kinda what templating languages do for you ;)
There was a problem hiding this comment.
Every caller will have to do that. I'd rather just prevent attr="undefined" in one place. Kinda what templating languages do for you ;)
I'd imagine most templating languages would give you title="undefined" in this case (which I assume is why the current partial has {{#if snippet}}title="{{snippet}}"{{/if}} in it). I'd generally consider accidentally passing in attributes with undefined values a bug and it would be nice to catch that
There was a problem hiding this comment.
Exactly. It's replacing that conditional. The good templating systems do this : http://jsbin.com/damayuhiwa/edit?html,output.
There was a problem hiding this comment.
I guess. You should document that behavior in that case so no one is surprised by it
| } | ||
|
|
||
| /** | ||
| * @param {!CardsDetailsJSON} details |
There was a problem hiding this comment.
render() needs to take a {(!DetailsRenderer.DetailsJSON|!DetailsRenderer.CardsDetailsJSON)} now...this is why we need to turn type checking back on :)
There was a problem hiding this comment.
done. closure is the worst.
There was a problem hiding this comment.
closure is the worst but not for this reason. The parameter can be one of two different things, so... :P
|
|
||
| /** | ||
| * @param {!DetailsJSON} details | ||
| * @param (!DetailsRenderer.DetailsJSON|!DetailsRenderer.CardsDetailsJSON)} details |
| } | ||
|
|
||
| /** | ||
| * @param {!CardsDetailsJSON} details |
There was a problem hiding this comment.
closure is the worst but not for this reason. The parameter can be one of two different things, so... :P
| } | ||
| Object.keys(attrs).forEach(key => { | ||
| element.setAttribute(key, attrs[key]); | ||
| if (attrs[key] !== undefined) { |
There was a problem hiding this comment.
Every caller will have to do that. I'd rather just prevent attr="undefined" in one place. Kinda what templating languages do for you ;)
I'd imagine most templating languages would give you title="undefined" in this case (which I assume is why the current partial has {{#if snippet}}title="{{snippet}}"{{/if}} in it). I'd generally consider accidentally passing in attributes with undefined values a bug and it would be nice to catch that
|
need a rebase now |
|
rebasing all my life. #K-Ci & JoJo |
R: all
This should wait until #1979 and #1987 land and will probably need rebasing, but review can start now. The changes are standalone from those.