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

report: add Util.prepareReportResult method #5766

Merged
merged 5 commits into from
Aug 7, 2018
Merged

Conversation

paulirish
Copy link
Member

@paulirish paulirish commented Aug 1, 2018

wdyt

currently depends on #5765

@paulirish paulirish added the i18n internationalization thangs label Aug 2, 2018
Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

Whoops, forgot to submit the review.

This LGTM, but could we not base it on #5765 for now so we can bikeshed on that one next week?

@@ -38,6 +38,39 @@ class Util {
return `%10d${NBSP}ms`;
}

/**
*
Copy link
Member

Choose a reason for hiding this comment

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

would be good to have a description, including that it returns a new copy of the results, doesn't mutate the original

Util.rendererFormattedStrings = clone.i18n.rendererFormattedStrings;
}

// TODO(phulce): we all agree this is technical debt we should fix
Copy link
Member

Choose a reason for hiding this comment

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

this should be moved up to cover the whole function IMO :)

@paulirish
Copy link
Member Author

This LGTM, but could we not base it on #5765 for now so we can bikeshed on that one next week?

sure. sg

so we can bikeshed on that one next week?

awwwwwwwww finnnnnne

@googlebot

This comment has been minimized.

@paulirish paulirish changed the base branch from rendererUIstringrefactor to master August 3, 2018 20:11
@googlebot
Copy link

CLAs look good, thanks!


// If LHR is older (≤3.0.3), it has no locale setting. Set default.
if (!clone.configSettings.locale) {
clone.configSettings.locale = 'en-US';
Copy link
Member

Choose a reason for hiding this comment

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

should this be en or do you want to fix in #5781 after this?


container.textContent = ''; // Remove previous report.
container.appendChild(this._renderReport(clone));
container.appendChild(this._renderReport(report));

// put the UIStrings back into original state
ReportRenderer.updateAllUIStrings(originalUIStrings);
Copy link
Member

Choose a reason for hiding this comment

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

Method should be on ReportRenderer or Util, but not both :)

Copy link
Member

Choose a reason for hiding this comment

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

It's pretty ugly that Util.prepareReportResult sets the UI strings but the caller is responsible for resetting them. Would it be acceptable to make two calls for users of this?

const report = Util.prepareReportResult(result);
Util.updateAllUIStrings(report.i18n.rendererFormattedStrings);

// ...

ReportRenderer.updateAllUIStrings(originalUIStrings);

(Util.prepareReportResult would have to at least set report.i18n.rendererFormattedStrings to a default empty object or the original UIStrings).

I guess on the other hand both proposed refactors have the string setting handled for the user of Util.prepareReportResult, so maybe it's fine to be ugly for now.

@@ -40,30 +40,17 @@ class ReportRenderer {
}

/**
* @param {LH.ReportResult} report
* @param {LH.ReportResult} result
Copy link
Member

Choose a reason for hiding this comment

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

really should be @param {LH.Result} result

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

LGTM!

@paulirish paulirish merged commit addd423 into master Aug 7, 2018
@paulirish paulirish deleted the prepareLHRmethod branch August 7, 2018 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
i18n internationalization thangs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants