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

i18n: perf-budget and res-summary column headers #9127

Merged
merged 5 commits into from
Jun 7, 2019
Merged

Conversation

exterkamp
Copy link
Member

Summary
Noticed this when snooping around for i18n stuff.

fixes: #9126

@exterkamp exterkamp added the i18n internationalization thangs label Jun 5, 2019
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 if we want to keep the Transfer Size label

lighthouse-core/audits/performance-budget.js Outdated Show resolved Hide resolved
@exterkamp exterkamp changed the title i18n: performance-budget column headers i18n: perf-budget and res-size column headers Jun 5, 2019
@exterkamp exterkamp changed the title i18n: perf-budget and res-size column headers i18n: perf-budget and res-summary column headers Jun 5, 2019
@exterkamp exterkamp requested a review from khempenius June 5, 2019 18:52
@@ -56,6 +56,12 @@ const UIStrings = {
columnWastedMs: 'Potential Savings',
/** Label for the time spent column in data tables, entries will be the number of milliseconds spent during a particular activity */
columnTimeSpent: 'Time Spent',
/** Label for the type of network resource column in data tables, entries will be enumerated kinds of network requests e.g. "Scripts", "Third-Party", "Stylesheet".*/
Copy link
Member

Choose a reason for hiding this comment

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

not sure when these descriptions switched formats above, but the ones below (like totalResourceType) seem like they'd read a lot clearer to someone with no context (like, what's a "type of network resource column in data tables"?).

e.g.
Label for a row in a data table; entries will be types of resources loaded over the network, e.g. "Scripts", "Third-Party", "Stylesheet".
seems better

Copy link
Member Author

Choose a reason for hiding this comment

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

The ones below are different. Those are rows in the perf-budgets table, not the columns. That's why they're different. But I agree they're more clear.

lighthouse-core/lib/i18n/i18n.js Outdated Show resolved Hide resolved
lighthouse-core/audits/performance-budget.js Outdated Show resolved Hide resolved
@@ -20,6 +20,8 @@ const UIStrings = {
=1 {1 request}
other {# requests}
}`,
/** Table column header for how displaying how much larger the size of network requests were than a predetermined budget. */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/** Table column header for how displaying how much larger the size of network requests were than a predetermined budget. */
/** Table column header displaying: how much greater the quantity of network requests were than a predetermined budget, how much larger the size of network requests were than a predetermined budget. */

Copy link
Member Author

Choose a reason for hiding this comment

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

Integrated this with the column format from i18n.js

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 with last thing

lighthouse-core/audits/performance-budget.js Outdated Show resolved Hide resolved
Co-Authored-By: Brendan Kenny <bckenny@gmail.com>
@googlebot

This comment has been minimized.

@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@googlebot

This comment has been minimized.

@googlebot googlebot added cla: no and removed cla: yes labels Jun 7, 2019
@googlebot

This comment has been minimized.

@exterkamp
Copy link
Member Author

Please Googlebot. Please.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes i18n internationalization thangs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

i18n, performance-budgets columns not translated
6 participants