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: placeholders #9114

Merged
merged 77 commits into from
Jul 24, 2019
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
77 commits
Select commit Hold shift + click to select a range
656acce
basic format
exterkamp Jun 3, 2019
fc762a5
Basic placeholder syntax.
exterkamp Jun 4, 2019
d1cdeab
minor corrections
exterkamp Jun 4, 2019
74e11c1
iterating
exterkamp Jun 4, 2019
981e028
Added ICU plural and ICU select examples.
exterkamp Jun 4, 2019
6d2dd5d
Gender example has replacement AND ICU.
exterkamp Jun 4, 2019
f22ee1b
less stutter syntax
exterkamp Jun 4, 2019
c66c0c6
docs
exterkamp Jun 4, 2019
70a5a07
Updated README.
exterkamp Jun 12, 2019
a7fc83a
correct-strings
exterkamp Jul 3, 2019
0de5485
updated i18n.js
exterkamp Jul 3, 2019
f8650eb
Merge branch 'master' into placeholders
exterkamp Jul 3, 2019
37dc2ae
remove unused from correct-strings
exterkamp Jul 3, 2019
58c7ded
exported correct-strings to be reused to import. ts first pass on co…
exterkamp Jul 3, 2019
ad01bdd
first pass at new UIStrings format.
exterkamp Jul 3, 2019
4182889
new jsdoc format for comments + _very_ basic parsing.
exterkamp Jul 4, 2019
8b0f8ea
Better parsing, filled out lh-error descriptions.
exterkamp Jul 5, 2019
ddf8171
Fixed tests, made unminified-css and doc-title back to normal.
exterkamp Jul 7, 2019
705ffb6
Updating readme for i18n.
exterkamp Jul 7, 2019
902c2f7
More readme updates.
exterkamp Jul 8, 2019
4507138
Move collect code into seperate util file for testing, and the tests …
exterkamp Jul 8, 2019
29e29df
Add placeholder baking tests.
exterkamp Jul 8, 2019
393beb6
readme updates
exterkamp Jul 8, 2019
799084c
More readme details.
exterkamp Jul 8, 2019
86e1593
Added test for ICU with examples, fixed audits that violated this.
exterkamp Jul 8, 2019
6160402
Patrick foodback
exterkamp Jul 9, 2019
02e8977
ICU select & ordinal readme examples. made runtime example more accu…
exterkamp Jul 9, 2019
f5083b8
collection-util docstrings
exterkamp Jul 9, 2019
5aca05a
remove old console log
exterkamp Jul 9, 2019
ee3e235
update assert on strings to new en-US
exterkamp Jul 9, 2019
6b8297d
polish
exterkamp Jul 9, 2019
77fcd92
refactoring conversion method, testing conversion to follow.
exterkamp Jul 9, 2019
7252f3d
brendan readme foodback.
exterkamp Jul 10, 2019
d4dd3ac
check for empty descriptions
exterkamp Jul 10, 2019
996ee5c
Use UIString as primary fallback, since it is just en. Update fallba…
exterkamp Jul 11, 2019
4ff414a
Untrack pre-locales.
exterkamp Jul 11, 2019
588f574
reorganized collect and correct
exterkamp Jul 11, 2019
2b17130
fixup collect-string output
paulirish Jul 11, 2019
570a6cc
Paul foodback. Readme updates, updated yarn cmds, updated ctc naming …
exterkamp Jul 12, 2019
dcc7cc7
Wording of CTC and LHL in readme and docstrings. Removed mkdirp. Ad…
exterkamp Jul 12, 2019
7743cda
rip my hopes and dreams, thanks linter.
exterkamp Jul 12, 2019
ce01c75
polish to package and to collect-strings
exterkamp Jul 12, 2019
dea280a
better examples!
exterkamp Jul 12, 2019
7f89ccf
correct some complex ICU, add backticks.
exterkamp Jul 15, 2019
cf42496
Merge branch 'master' into placeholders
exterkamp Jul 15, 2019
d156737
Update 'yarn update:sample-json' and json itself
exterkamp Jul 15, 2019
89410f8
Handle the TODOs in readme.
exterkamp Jul 15, 2019
ea3616b
Added @meanings for all duplicated strings.
exterkamp Jul 15, 2019
3d40437
update sample json
exterkamp Jul 15, 2019
bc2b8f3
remove from UIStrings, add autofilled meanings + warning log.
exterkamp Jul 16, 2019
92cafeb
Some comments
exterkamp Jul 16, 2019
3d746d6
added TODO to collect-strings
exterkamp Jul 16, 2019
5ad0dff
Merge branch 'master' into placeholders
exterkamp Jul 17, 2019
c2e4ce5
Paul foodback.
exterkamp Jul 17, 2019
5dd3e07
Brendan baking foodback part 1.
exterkamp Jul 18, 2019
6eef366
update bakery, and tests. Remove space from yarn clean.
exterkamp Jul 18, 2019
a35a45e
lint
exterkamp Jul 18, 2019
13d0f02
foodback round 2, renaming party.
exterkamp Jul 18, 2019
b34ef95
Last part of feedback
exterkamp Jul 18, 2019
48283f4
windows compat collect-strings handled in appropriate PR.
exterkamp Jul 18, 2019
95527d5
Brendan foodback part 2.
exterkamp Jul 19, 2019
327c7db
How'd that var get there.
exterkamp Jul 19, 2019
b006cf7
Brendan feedback v3
exterkamp Jul 21, 2019
51cbb78
added comment to example fallback.
exterkamp Jul 22, 2019
37be64c
Merge branch 'master' into placeholders
exterkamp Jul 22, 2019
d2a8326
complex ICU -> custom-formatted ICU
exterkamp Jul 22, 2019
be426b8
reorg example logic to directIcu replacement.
exterkamp Jul 22, 2019
125cb1d
stop empty plcaeholders from being added to every message. Stricter c…
exterkamp Jul 22, 2019
b95b96b
updated columnName description
exterkamp Jul 22, 2019
90efcff
actually can't assert that in the tests. The undefined part is done …
exterkamp Jul 22, 2019
6e6ef24
@example {example} varName
exterkamp Jul 23, 2019
6a9ef28
whoops. console log left in
exterkamp Jul 23, 2019
7762381
brendan feedback
exterkamp Jul 23, 2019
6c19a17
Merge branch 'master' into placeholders
exterkamp Jul 24, 2019
be34f7e
Add sanity check for ctc files
exterkamp Jul 24, 2019
23a68d5
I feel like my linter is turned off
exterkamp Jul 24, 2019
3685dd6
Merge branch 'master' into placeholders
exterkamp Jul 24, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions lighthouse-core/audits/accessibility/document-title.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,15 @@ const AxeAudit = require('./axe-audit.js');
const i18n = require('../../lib/i18n/i18n.js');

const UIStrings = {
/** Title of an accesibility audit that evaluates if the page has a <title> element that describes the page. This title is descriptive of the successful state and is shown to users when no user action is required. */
title: 'Document has a `<title>` element',
title: {
message: 'Document has a $html_title$ element',
description: 'Title of an accesibility audit that evaluates if the page has a <title> element that describes the page. This title is descriptive of the successful state and is shown to users when no user action is required.',
placeholders: {
html_title: {
content: '`<title>`',
},
},
},
/** Title of an accesibility audit that evaluates if the page has a <title> element that describes the page. This title is descriptive of the failing state and is shown to users when there is a failure that needs to be addressed. */
failureTitle: 'Document doesn\'t have a `<title>` element',
/** Description of a Lighthouse audit that tells the user *why* they should try to pass. This is displayed after a user expands the section to see more. No character length limits. 'Learn More' becomes link text to additional documentation. */
Expand Down
17 changes: 15 additions & 2 deletions lighthouse-core/audits/byte-efficiency/unminified-css.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,21 @@ const UIStrings = {
/** Imperative title of a Lighthouse audit that tells the user to minify (remove whitespace) the page's CSS code. This is displayed in a list of audit titles that Lighthouse generates. */
title: 'Minify CSS',
/** Description of a Lighthouse audit that tells the user *why* they should minify (remove whitespace) the page's CSS code. This is displayed after a user expands the section to see more. No character length limits. 'Learn More' becomes link text to additional documentation. */
description: 'Minifying CSS files can reduce network payload sizes. ' +
'[Learn more](https://developers.google.com/web/tools/lighthouse/audits/minify-css).',
description: {
message: 'Minifying CSS files can reduce network payload sizes.' +
' $link_start$Learn More$link_end$',
description: 'Description of a Lighthouse audit that tells the user *why* they should ' +
'minify (remove whitespace) the page\'s CSS code. This is displayed after a user expands ' +
'the section to see more. No character length limits.',
placeholders: {
link_start: {
content: '[',
},
link_end: {
content: '](https://developers.google.com/web/tools/lighthouse/audits/minify-css)',
Copy link
Member

Choose a reason for hiding this comment

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

this is a pretty unfortunate syntax. Maybe we want to come up with something that expands to this in collect-strings.js?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we want to come up with something that expands to this in collect-strings.js?

Just to be clear, I meant this as a super low priority idea that we can think about while we do everything else first and implement it later if there's a nice fix. It kind of sucks to look at/edit but we could live with it if need be (we may need a test to make sure a typo in one of the contents doesn't accidentally break a link)

Copy link
Member Author

Choose a reason for hiding this comment

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

That was also my thought. This is super heavyweight to have in each .js file. I can try to experiment.

Copy link
Collaborator

@connorjclark connorjclark Jun 4, 2019

Choose a reason for hiding this comment

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

something that expands to this in collect-strings.js

Let's just keep it as [some text {name} or even placeholders](url) and expand it to additional placeholders in collect strings.

Copy link
Member Author

@exterkamp exterkamp Jun 4, 2019

Choose a reason for hiding this comment

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

Like auto convert [ and ](url) into link_start link_end in collect strings?

I worry about making special cases, adding unseen complexity behind the scenes. I think part of the positives of a change like this is that it removes all ambiguity.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah - I think that'd be dope

},
},
},
};

const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings);
Expand Down
21 changes: 17 additions & 4 deletions lighthouse-core/lib/i18n/en-US.json
Original file line number Diff line number Diff line change
Expand Up @@ -180,8 +180,13 @@
"description": "Title of an accesibility audit that evaluates if the page has a <title> element that describes the page. This title is descriptive of the failing state and is shown to users when there is a failure that needs to be addressed."
},
"lighthouse-core/audits/accessibility/document-title.js | title": {
"message": "Document has a `<title>` element",
"description": "Title of an accesibility audit that evaluates if the page has a <title> element that describes the page. This title is descriptive of the successful state and is shown to users when no user action is required."
"message": "Document has a $html_title$ element",
"description": "Title of an accesibility audit that evaluates if the page has a <title> element that describes the page. This title is descriptive of the successful state and is shown to users when no user action is required.",
"placeholders": {
"html_title": {
"content": "`<title>`"
}
}
},
"lighthouse-core/audits/accessibility/duplicate-id.js | description": {
"message": "The value of an id attribute must be unique to prevent other instances from being overlooked by assistive technologies. [Learn more](https://dequeuniversity.com/rules/axe/3.1/duplicate-id?application=lighthouse).",
Expand Down Expand Up @@ -508,8 +513,16 @@
"description": "Title of a diagnostic audit that provides detail on large network resources required during page load. 'Payloads' is roughly equivalent to 'resources'. This descriptive title is shown to users when the amount is acceptable and no user action is required."
},
"lighthouse-core/audits/byte-efficiency/unminified-css.js | description": {
"message": "Minifying CSS files can reduce network payload sizes. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/minify-css).",
"description": "Description of a Lighthouse audit that tells the user *why* they should minify (remove whitespace) the page's CSS code. This is displayed after a user expands the section to see more. No character length limits. 'Learn More' becomes link text to additional documentation."
"message": "Minifying CSS files can reduce network payload sizes. $link_start$Learn More$link_end$",
"description": "Description of a Lighthouse audit that tells the user *why* they should minify (remove whitespace) the page's CSS code. This is displayed after a user expands the section to see more. No character length limits.",
"placeholders": {
"link_start": {
"content": "["
},
"link_end": {
"content": "](https://developers.google.com/web/tools/lighthouse/audits/minify-css)"
}
}
},
"lighthouse-core/audits/byte-efficiency/unminified-css.js | title": {
"message": "Minify CSS",
Expand Down
18 changes: 17 additions & 1 deletion lighthouse-core/lib/i18n/i18n.js
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,23 @@ const _icuMessageInstanceMap = new Map();
*/
function _formatIcuMessage(locale, icuMessageId, icuMessage, values) {
const localeMessages = LOCALES[locale];
const localeMessage = localeMessages[icuMessageId] && localeMessages[icuMessageId].message;
let localeMessage = localeMessages[icuMessageId] && localeMessages[icuMessageId].message;
// lets try to replace some things :o
const placeholders = localeMessages[icuMessageId] && localeMessages[icuMessageId].placeholders;
if (placeholders) {
console.log('placeholders!');
// do some regex
Object.entries(placeholders).forEach(entry => {
const key = entry[0];
const value = entry[1];
//use key and value here
let regexStr = `\$${key}\$`;
Copy link
Member

Choose a reason for hiding this comment

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

I think we'll definitely want a collect-strings test that verifies that all the placeholders are in the string (would we also want the stricter check that there aren't any escaped values that don't have placeholders?)

console.log(key, value, regexStr);
localeMessage = localeMessage.replace(regexStr, value.content);
Copy link
Member

Choose a reason for hiding this comment

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

maybe to avoid the issue with positional replacements ($1 etc), we could use use the named replacements in values if value.content is \$\d+ or whatever? We'll just have to figure out how to do the mapping (just use key?)

console.log(localeMessage);
});
}

// fallback to the original english message if we couldn't find a message in the specified locale
// better to have an english message than no message at all, in some number cases it won't even matter
const messageForMessageFormat = localeMessage || icuMessage;
Expand Down
16 changes: 12 additions & 4 deletions lighthouse-core/scripts/i18n/collect-strings.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,10 +84,18 @@ function collectAllStringsInDir(dir, strings = {}) {
let lastPropertyEndIndex = 0;
for (const property of stmt.declarations[0].init.properties) {
const key = property.key.name;
const message = exportVars.UIStrings[key];
const description = computeDescription(ast, property, lastPropertyEndIndex);
strings[`${relativePath} | ${key}`] = {message, description};
lastPropertyEndIndex = property.range[1];
const val = exportVars.UIStrings[key];
if (typeof val === 'string') {
const description = computeDescription(ast, property, lastPropertyEndIndex);
strings[`${relativePath} | ${key}`] = {message: val, description};
lastPropertyEndIndex = property.range[1];
} else {
const message = val.message;
const description = val.description;
const placeholders = val.placeholders;
strings[`${relativePath} | ${key}`] = {message, description, placeholders};
lastPropertyEndIndex = property.range[1];
}
}
}
}
Expand Down