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(report): runtime settings and tools #9166

Merged
merged 32 commits into from
Jan 14, 2020
Merged

i18n(report): runtime settings and tools #9166

merged 32 commits into from
Jan 14, 2020

Conversation

exterkamp
Copy link
Member

@exterkamp exterkamp commented Jun 10, 2019

Summary
i18n runtime settings (static strings only) and dropdown tools. Also modify report-ui-features so that it i18n's it's strings (third-party-resources wasn't actually i18n'ing).

Notes:

  • Since Util uses the static-ish replacement strings it isn't currently possible to do ICU replacements ({var} syntax) in order to incorporate strings like 150 ms TCP RTT, 1,638.4 Kbps throughput (Simulated) properly, e.g. {timeInMs, number, milliseconds} TCP RTT, {throughput, number, bytes} throughput (Simulated). So they are left as-is in this PR, with a follow up TODO.
  • The footer that has the Version concatenated onto it Generated by Lighthouse 5.1.0 needs to be an ICU replacement as well, but for the same reason cannot be 😦, so I added a TODO.
  • Summary is no longer used in runtime settings post 5.x report renderers
  • Updating the reportStrings in proto is a crime. Who would possibly think proto was a good idea?

Screenshot examples of i18n'd runtime settings in ja (google translate translations, they probably don't really make a ton of sense in Japanese)

  • Runtime Settings: image
  • Dropdown: image
  • 3rd party res: image

Related Issues/PRs
part of: #7238

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.

nice @exterkamp!!

lighthouse-core/report/html/renderer/util.js Outdated Show resolved Hide resolved
dropdownSaveHTML: 'Save as HTML',
/** Option in a dropdown menu that save the Lighthouse JSON object to the local system as a '.json' file. */
dropdownSaveJSON: 'Save as JSON',
/** Option in a dropdown menu that opens the current report in the Lighthouse Viewer Application. */
Copy link
Collaborator

Choose a reason for hiding this comment

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

LVA, I like it 😎

lighthouse-core/report/html/renderer/util.js Outdated Show resolved Hide resolved
lighthouse-core/report/html/renderer/util.js Outdated Show resolved Hide resolved
lighthouse-core/report/html/renderer/util.js Outdated Show resolved Hide resolved
@@ -418,69 +418,67 @@ class Util {

return [
{
name: 'Device',
name: this.UIStrings.runtimeSettingsDevice,
Copy link
Collaborator

Choose a reason for hiding this comment

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

any reason we need to use this instead of Util here?

Copy link
Member Author

Choose a reason for hiding this comment

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

No not really, I was following the prior art for that file, but using this. for only this file seems bad.


return {
deviceEmulation,
cpuThrottling,
networkThrottling,
summary: `${deviceEmulation}, ${summary}`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

did we just not use this? 😆

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh yeah forgot to mention, we removed this in the new report redesign. It was on the top header, but no more!

@exterkamp
Copy link
Member Author

en-XL preview of what is and isn't translated in this PR
image
image
image

Changed collision count to 17 for util collisions. Improved how
collect-strings surfaces collisions in error msg.
@exterkamp
Copy link
Member Author

en-XL 🎉
image

Co-Authored-By: Connor Clark <cjamcl@google.com>
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

/** Descriptive explanation for emulation setting when no device emulation is set. */
runtimeNoEmulation: 'No emulation',
/** Descriptive explanation for emulation setting when emulating a Nexus 5X mobile device. */
runtimeMobileEmulation: 'Emulated Nexus 5X',
Copy link
Collaborator

Choose a reason for hiding this comment

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

heads up Moto G4 conflict ahead :) #10191

lighthouse-core/report/html/templates.html Outdated Show resolved Hide resolved
lighthouse-core/scripts/i18n/collect-strings.js Outdated Show resolved Hide resolved
@patrickhulce
Copy link
Collaborator

@exterkamp FYI there appear to be axe-related unit test failures on the report

@exterkamp
Copy link
Member Author

exterkamp commented Jan 14, 2020

Removing the text from the dropdown caused the axe failures, I ignored that test in axe. And I updated the snapshots + the minification test that was failing.

Nevermind all that snapshotting stuff, my repo was messed up. The ignoring of link-name was all that I needed.

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

Successfully merging this pull request may close these issues.

6 participants