Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: seo #6860
i18n: seo #6860
Changes from 2 commits
31d658b
8c79982
c8b9f58
42d8dda
60fc6b9
b06fd1f
8306ee7
2c8eea0
ffa4e7c
0570900
f5b7751
7f47810
5b77efe
3292058
b839a63
0f23dee
cf3ed80
feea7c3
93a99bf
7b3fcb5
aa9bb0b
62564d5
6dfbd5c
18a75ee
324db81
5926722
10cafea
7294f93
edea4fb
b122256
2d36b31
4b692fa
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is still a URL, so may be good to use the same
url
replacement term as the other explanations (will need to update the description if changing this one)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this should still always be a url. Good point!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
{disclaimer}
here is nested and will not be translated. Should we look into modifying i18n to have nested translations or combinations?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this (combining) would require a few additions elsewhere in the i18n handling right? (this is where I admit I don't know how i18n works here yet)
That, or have two separate UIStrings for cases w/ the disclaimer and w/o.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'll have to do
str_(UIStrings.explanation, {decimalProportion) + ' ' + str_(UIStrings.disclaimer, {percentage})
it's not super kosher but it's better than having a random english string
the first argument to
str_
must always be a statically known valueThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The i18n replacement in the end can't replace 2 lookup strings can it, the output ends up like this afaik:
"lighthouse-core/audits/seo/font-size.js | explanation # 1 lighthouse-core/audits/seo/font-size.js | disclaimer # 0."
b/c it can't lookup 2 replacements in 1 string? Should we add that, or is there an option I'm not setting/seeing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right we defer them to the end now, this is gonna be tricky 🤔 we might want to come up with a new format to handle this type of thing, like an
I18NCombinedString
object that gets replaced instead of just a normal stringThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to some concat string handling (not nesting :).
As long as the constituent strings are findable statically, the human translators will understand and replacement should work fine after concatenating. We just need to get i18n.js to handle it without adding too much complexity.
Or what's the combinatorics on this audit? We could just defer on this and include multiple (partially redundant) explanation UIStrings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just two. I vote for deferring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deferred by splitting into explanation with disclaimer and without
punted 🏈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are there any docs explaining how this format works
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
http://userguide.icu-project.org/formatparse/messages#TOC-Complex-Argument-Types