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(import): new audits, dropdowns, and columns #10645

Merged
merged 7 commits into from
May 11, 2020

Conversation

exterkamp
Copy link
Member

Summary
Automated import of strings.

Related Issues/PRs
fixes: #10581

@exterkamp exterkamp requested a review from a team as a code owner April 27, 2020 21:07
@exterkamp exterkamp requested review from patrickhulce and removed request for a team April 27, 2020 21:07
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.

I think tests are complaining about the same comment I had.

Not sure what other role I have in approving this :)

"lighthouse-core/audits/metrics/cumulative-layout-shift.js | description": {
"message": "Kumulativní změna rozvržení je součet všech změn rozvržení, k nimž došlo při načítání stránky. Změna rozvržení je libovolná změna pozice, k níž u prvku dojde poté, co je viditelný uživateli. Všechny změny rozvržení se zaznamenávají a přiděluje se jim skóre. Výsledné kumulativní skóre má hodnotu mezi 0 a 1. 0 znamená perfektně stabilní stránku, skóre >= 0,5 znamená vysoce proměnlivou stránku. [Další informace](https://web.dev/cls)"
},
"lighthouse-core/audits/metrics/cumulative-layout-shift.js | title": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

isn't this out-of-date already? 😬 do we need another import to get the correct one for the move to i18n.js cumulativeLayoutShiftMetric?

Copy link
Collaborator

Choose a reason for hiding this comment

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

hehehehe

Copy link
Collaborator

Choose a reason for hiding this comment

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

@exterkamp can we immediately do another import just to get the default locale here fixed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

poor translators, I bet the description was a lot of work

Copy link
Member Author

Choose a reason for hiding this comment

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

@exterkamp can we immediately do another import just to get the default locale here fixed?

Yes. But an automated import will reset all languages to English for that string. So. We can manually commit the new, correct English one and export everything at the same time. This will reset the default, and start translating the new one, without poisoning the translated ones.

Are these ones just so bad that we shouldn't commit them, or that they just aren't perfect and need to be redone immediately (but we could still land this)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there are two separate issues going on here @exterkamp.

  1. The titles of CLS and LCP were moved out of the metric files into i18n.js. The string didn't change so translations should be fine, but the key did and AFAICT in all languages we'll be using the english name for CLS and LCP right now which would be pretty bad for 6.0 IMO.
  2. The description of CLS changed to be shorter. I see no need to force a retranslation of all of these before 6.0 if we can still get the longer translated version in. It seems fine if it lags a little bit.

I am mostly concerned about the first bullet.

Copy link
Member

Choose a reason for hiding this comment

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

The titles of CLS and LCP were moved out of the metric files into i18n.js. The string didn't change so translations should be fine, but the key did and AFAICT in all languages we'll be using the english name for CLS and LCP right now which would be pretty bad for 6.0 IMO.

Since the string didn't change, just the key, and we don't send the key to the translators, will these insta-translate if we resubmit, @exterkamp?

Copy link
Collaborator

Choose a reason for hiding this comment

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

⬆️ THIS! :)

Thanks @brendankenny for saying what I was horribly trying to get at with "do we need another import to get the correct one" 😆

Copy link
Collaborator

Choose a reason for hiding this comment

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

i think we ask @exterkamp this once a quarter. can we write the answer in the docs

@brendankenny
Copy link
Member

So I think the plan now is to

  • land report: add Trust & Safety group under Best Practices #10623 (to get new Best Practices group strings)
  • re-import strings to TC
  • get a same-day string dump that will have CLS and LCP titles in their new location
  • update this PR with those strings (and merge)
  • do another string PR when the new strings (CLS description, Best Practices group titles) are ready

?

@paulirish
Copy link
Member

So I think the plan now is to

^ done done done done

  • (and merge)
  • do another string PR when the new strings (CLS description, Best Practices group titles) are ready

^ not done.

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, though strings are expensive in the lr bundle, huh

@@ -630,7 +648,7 @@
"message": "Für Nutzer mit langsamen Verbindungen können externe Skripts, die dynamisch über `document.write()` eingefügt werden, den Seitenaufbau um einige Sekunden verzögern. [Weitere Informationen.](https://web.dev/no-document-write)"
},
"lighthouse-core/audits/dobetterweb/no-document-write.js | failureTitle": {
"message": "Verwendet `document.write()`"
"message": "Avoid `document.write()`"
Copy link
Member

Choose a reason for hiding this comment

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

one weird thing is that all the languages adopted the english string for the document.write failureTitle, which doesn't seem like the correct behavior? I would assume it would have been dropped since there wasn't a new translation

@exterkamp

Copy link
Member

@paulirish paulirish May 7, 2020

Choose a reason for hiding this comment

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

Yeah I did see these removals:

image

but the lack of removal for these others is a lil weird.

Copy link
Member

Choose a reason for hiding this comment

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

Shane clarified that we only 'remove messages' when its not part of our source files. (not in the ctc)

We don't remove messages that are not-yet-translated. And they are deliberately populated with the fallback (en) strings.

@paulirish
Copy link
Member

paulirish commented May 8, 2020

Latest export all the recent translations of our document.write wording tweak. We're at 100% now.

I'll merge now.

@patrickhulce
Copy link
Collaborator

Bot isn't working, so merging for Paul.

@patrickhulce patrickhulce merged commit d95c31b into master May 11, 2020
@patrickhulce patrickhulce deleted the import-lr-translations-04-20 branch May 11, 2020 15:04
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.

Import latest translated strings
7 participants