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

core(deprecations): use translated strings from devtools repo #13961

Merged
merged 10 commits into from
Jul 21, 2022

Conversation

connorjclark
Copy link
Collaborator

Fixes #13895

Putting this up, open question about what to do with all the missing translator strings. Wait, or fill them out ourselves?

geolocationInsecureOriginDeprecatedNotRemoved:
'`getCurrentPosition()` and `watchPosition()` are deprecated on insecure origins. To use this feature, you should consider switching your application to a secure origin, such as HTTPS. See https://goo.gle/chrome-insecure-origins for more details.',
/**
*@description TODO(crbug.com/1318858): Description needed for translation
Copy link
Member

Choose a reason for hiding this comment

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

Will translators still attempt to translate these strings without a description?

Copy link
Member

Choose a reason for hiding this comment

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

When translators ask us why there isn't a context description, are we going to have to tell them that it's it not our fault, we got these from somewhere else.

Or in theory should that never happen, because these specific strings will be referred to the existing translations from DT.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Or in theory should that never happen, because these specific strings will be referred to the existing translations from DT.

This

switch (deprecation.type) {
case 'AuthorizationCoveredByWildcard':
message = str_(UIStrings.authorizationCoveredByWildcard);
milestone = 97;
Copy link
Member

Choose a reason for hiding this comment

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

If deprecations are added/changed we are going to need to update this like InspectorIssues right?

Copy link
Member

Choose a reason for hiding this comment

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

Should we have a CI check like inspector issues to force us to keep this updated?

@paulirish
Copy link
Member

Whats the process you used to slurp up the strings and the feature/milestone numbers?
Clearly we'll have to regularly update this, along with our other two "syncable" things (installability and inspector-issues).

@connorjclark
Copy link
Collaborator Author

No process. I copied the function from devtools.

@connorjclark
Copy link
Collaborator Author

I added a minor build step to make updating these string simple in the future.

@paulirish, would third-party/chromium-synchronization/inspector-issueAdded-types-test.js be a good test to amend for making sure we have coverage of all these deprecation types? Or instead, we could check that we have the latest commit message of DeprecationIssue.ts (we'd hardcode the commit sha in the test file).

@connorjclark connorjclark changed the title wip core(deprecations): use translated strings core(deprecations): use translated strings Jun 9, 2022
@connorjclark connorjclark changed the title core(deprecations): use translated strings core(deprecations): use translated strings from devtools repo Jun 9, 2022
@connorjclark connorjclark marked this pull request as ready for review June 9, 2022 00:47
@connorjclark
Copy link
Collaborator Author

todo: need to fix build-cdt-lib..

Error: did not find expected code fragment: url += '? [sm]'
    at file:///Users/cjamcl/src/lighthouse/build/build-cdt-lib.js:104:13

"message": "{PH1} is deprecated. Please use {PH2} instead."
},
"lighthouse-core/audits/deprecations-strings.js | deprecationExample": {
"message": "This is an example of a translated deprecation issue message."
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

uhhh


/**
* @param {string} text
* @param {string} needle
Copy link
Member

Choose a reason for hiding this comment

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

Is needle common nomenclature for this? I would prefer startPattern and endPattern

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

indeed, I have seen needle as a term for replacements in many places.

geolocationInsecureOriginDeprecatedNotRemoved:
'`getCurrentPosition()` and `watchPosition()` are deprecated on insecure origins. To use this feature, you should consider switching your application to a secure origin, such as HTTPS. See https://goo.gle/chrome-insecure-origins for more details.',
/**
*@description TODO(crbug.com/1318858): Description needed for translation
Copy link
Member

Choose a reason for hiding this comment

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

When translators ask us why there isn't a context description, are we going to have to tell them that it's it not our fault, we got these from somewhere else.

Or in theory should that never happen, because these specific strings will be referred to the existing translations from DT.

Copy link
Member

@adamraine adamraine left a comment

Choose a reason for hiding this comment

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

I'll defer to you on the CI check, but otherwise LGTM

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.

InspectorIssues protocol no longer contains message strings for deprecations
4 participants