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(flow): import lhr strings #13215

Merged
merged 15 commits into from
Oct 18, 2021
Merged

report(flow): import lhr strings #13215

merged 15 commits into from
Oct 18, 2021

Conversation

adamraine
Copy link
Member

The report is borked if a navigation category has null score because Util.i18n is not initialized when rendering category gauges on the summary page.

This PR imports the UI strings from the first lhr and uses them to initialize Util.i18n.

#11313

@adamraine adamraine requested a review from a team as a code owner October 14, 2021 22:05
@adamraine adamraine requested review from connorjclark and removed request for a team October 14, 2021 22:05
@google-cla google-cla bot added the cla: yes label Oct 14, 2021
Comment on lines +45 to +47
settings.formFactor === 'desktop' ?
strings.runtimeDesktopEmulation :
strings.runtimeMobileEmulation
Copy link
Member Author

Choose a reason for hiding this comment

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

We can now use lhr strings pretty easily.

Copy link
Member

Choose a reason for hiding this comment

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

We can now use lhr strings pretty easily.

this is great

flow-report/src/i18n/i18n.tsx Outdated Show resolved Hide resolved
flow-report/src/i18n/i18n.tsx Outdated Show resolved Hide resolved
flow-report/src/i18n/i18n.tsx Outdated Show resolved Hide resolved
flow-report/src/i18n/i18n.tsx Outdated Show resolved Hide resolved
flow-report/src/sidebar/sidebar.tsx Show resolved Hide resolved
Comment on lines +45 to +47
settings.formFactor === 'desktop' ?
strings.runtimeDesktopEmulation :
strings.runtimeMobileEmulation
Copy link
Member

Choose a reason for hiding this comment

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

We can now use lhr strings pretty easily.

this is great

flow-report/src/summary/category.tsx Show resolved Hide resolved
// Set missing flow strings to default (english) values.
...UIStrings,
// `strings` is generated in build/build-report.js
...strings[locale],
Copy link
Member

Choose a reason for hiding this comment

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

how hard would it be to test this fallback mechanism? Delete a string from a locale, load a report with that locale, check that all the strings are present?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's possible. Most annoying part would probably be mocking the localized-strings.js file.

Copy link
Member

@brendankenny brendankenny Oct 15, 2021

Choose a reason for hiding this comment

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

I was just trying to think of how we can have a test catch regressions in this system, because even when you manually check a report in one language you may not notice a single missing string, and we don't look at reports in other locales all that often.

Since the flow report strings are all known at build time (unlike the report strings, which depend partly on what's in the LHR), another option is to just go through all the locales and check that all the strings are there. When you pose it that way it kind of reveals itself as not that strong of a test, but it would be exhaustive, it should still be pretty fast, and it wouldn't require mocking :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add an item to #11313

@adamraine
Copy link
Member Author

Looks like static i18n = null is causing issues for yarn build-devtools. Not sure exactly why, but I think reverting to the @ts-ignore will be fine for now if we plan to remove i18n from Util.

@adamraine adamraine merged commit bbb2d67 into master Oct 18, 2021
@adamraine adamraine deleted the flow-import-lhr-strings branch October 18, 2021 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants