Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core(tsc): polish switch to new audit-details types #7285

Merged
merged 3 commits into from
Feb 21, 2019
Merged

Conversation

brendankenny
Copy link
Member

Removes a few leftover TODOs and @ts-ignores and removed the middle man on some of the details types being wrapped in an object and then immediately unwrapped again.

With this PR the LH.Audit.Details fix process should be finished.

@@ -447,7 +447,6 @@ class ReportUIFeatures {
*/
getReportHtml() {
this._resetUIState();
// @ts-ignore - technically documentElement can be null, but that's dumb - https://dom.spec.whatwg.org/#document-element
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was unrelated, but noticed it when I was searching for @ts-ignore. Fixed in some tsc release since the comment was added.

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -151,8 +151,7 @@ class UnusedCSSRules extends ByteEfficiencyAudit {
}

const usage = UnusedCSSRules.computeUsage(stylesheetInfo);
// @ts-ignore TODO(bckenny): fix index signature on ByteEfficiencyItem.
return Object.assign({url}, usage);
return {url, ...usage};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😆

@@ -207,7 +201,7 @@ class DetailsRenderer {
// The value's type overrides the heading's for this column.
switch (value.type) {
case 'code': {
return this._renderCode(value);
return this._renderCode(value.value);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 not that it's new, but seems like we need a new local name for value

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but seems like we need a new local name for value

haha, yeah. I actually like the local value and would rather change the property (e.g. value.code), but that's probably way too much for now :)

@brendankenny brendankenny merged commit a529731 into master Feb 21, 2019
@brendankenny brendankenny deleted the finaltweaks branch February 21, 2019 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants