i18n: seo #6860
i18n: seo #6860
Conversation
exterkamp
requested review from
paulirish,
brendankenny,
patrickhulce and
connorjclark
|
|
||
| explanation = `${percentageOfFailingText}% of text is too small${disclaimer}.`; | ||
| explanation = str_(UIStrings.explanation, | ||
| {decimalProportion: percentageOfFailingText, disclaimer}); |
There was a problem hiding this comment.
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.
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)
if (visitedTextLength < totalTextLength) {
const percentageOfVisitedText = visitedTextLength / totalTextLength * 100;
disclaimer = str_(UIStrings.explanation, {percentage: percentageOfVisitedText.toFixed()})
}
explanation = str_(UIStrings.explanation,
{decimalProportion: percentageOfFailingText} + disclaimer)
That, or have two separate UIStrings for cases w/ the disclaimer and w/o.
There was a problem hiding this comment.
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 value
There was a problem hiding this comment.
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.
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 string
There was a problem hiding this comment.
+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.
Or what's the combinatorics on this audit?
It's just two. I vote for deferring.
There was a problem hiding this comment.
Deferred by splitting into explanation with disclaimer and without
punted 🏈
There was a problem hiding this comment.
for these titles that describe the state like a11y I went with
This title is descriptive of the successful state and is shown to users when no user action is required.
and
This title is descriptive of the failing state and is shown to users when there is a failure that needs to be addressed.
after the basic outline of what the audit did, maybe we stick with that or come up with a new strategy? the only imperative ones should be the audit opportunities, so if it was copy/paste we probably need to fix some more audits out there :)
| const UIStrings = { | ||
| /** Title of a Lighthouse audit that tells the user their links have descriptive enough text for a search engine crawler to parse them. This is displayed in a list of audit titles that Lighthouse generates. */ | ||
| title: 'Links have descriptive text', | ||
| /** Title of a Lighthouse audit that tells the user their links have descriptive enough text for a search engine crawler to parse them. This title is shown when there are links on the page without proper text to describe them. */ |
There was a problem hiding this comment.
I'm not sure if this pattern is followed elsewhere, but I personally find it a tad unclear what's going on after the first sentence since they're the same.
IMO we could flip the first sentence Title of a Lighthouse audit that tells the user their links **do not** have descriptive enough text for a search engine crawler to parse them. and then we don't need the 2nd or the 2nd could explain that it's showed in a list of other failing audit titles that lighthouse generates?
There was a problem hiding this comment.
I guess it depends on how you interpret the that clause, if it modifies audit I get it, if it modifies Title then it's not true :)
There was a problem hiding this comment.
We do this a lot where the first sentence is shared between the title and failureTitle, but other places we don't. When I initially looked I thought this was the standard, but it seems mixed. So I'm not sure if we want to standardize one way or the other, or move forward this was or that?
lighthouse/lighthouse-core/audits/dobetterweb/dom-size.js
Lines 24 to 27 in 4f16a6a
lighthouse/lighthouse-core/audits/time-to-first-byte.js
Lines 13 to 16 in 4f16a6a
lighthouse/lighthouse-core/audits/bootup-time.js
Lines 16 to 19 in 4f16a6a
There was a problem hiding this comment.
Oh gotcha yeah if we change to match their style. i.e. that provides detail on is the first sentence and the conditions that describe pass/fail in the second sentence, I think that makes a lot more sense 👍
The "that tells the user the links have descriptive enough text" part when they don't have enough descriptive text seems like a recipe for translator confusion
There was a problem hiding this comment.
Took another shot at it with the that provides detail on styling. I think it is much cleaner!
|
I'm gonna make sure that the category title + description are there. Don't commit this yet. |
|
Does this mean you're ready for more review now @exterkamp ? |
|
Yup ready for review! |
There was a problem hiding this comment.
nice @exterkamp great job on the descriptions I agree they are much easier to grok now!
I know this is all pretty tedious stuff too we're almost there 😆
| '[Learn more](https://developers.google.com/web/tools/lighthouse/audits/descriptive-link-text).', | ||
| /** [ICU Syntax] Label for the audit identifying the number of links found. */ | ||
| displayValue: `{itemCount, plural, | ||
| =1 {1 link found} |
There was a problem hiding this comment.
are there any docs explaining how this format works
There was a problem hiding this comment.
There was a problem hiding this comment.
These comments are all on one line so github suggestions work great for them :)
Don't feel obligated to accept suggestions as they are, they're more to clarify some wording and adding qualifiers or additional descriptions. I also wrote them quite quickly :) Incorporate or reword some more.
I think this is almost there. Let's land it!
| const i18n = require('../../lib/i18n/i18n.js'); | ||
|
|
||
| const UIStrings = { | ||
| /** Title of a Lighthouse audit that provides detail on the robots.txt file on the page. This descriptive title is shown when the robots.txt file is present and configured correctly. */ |
There was a problem hiding this comment.
"on the page" doesn't really make sense here and the failureTitle below. "on the site's robots.txt file" maybe? Maybe also leave a note to not translate "robots.txt" (and we can hopefully do #6945 for real later)
| description: 'If your robots.txt file is malformed, crawlers may not be able to understand ' + | ||
| 'how you want your website to be crawled or indexed.', | ||
| /** [ICU Syntax] Label for the audit identifying that the robots.txt request has returned a specific HTTP status code. */ | ||
| displayValueHttpError: 'request for robots.txt returned HTTP status: {statusCode, number}', |
There was a problem hiding this comment.
maybe switch around the variable name for this (and the next one) to httpErrorDisplayValue to differentiate from lhErrors and so it's clear the thing itself isn't an error or error message, it's the display value when there was an error.
Also give more context for the variable. "statusCode will be replaced with a three-digit integer which represents the status of the HTTP connection (e.g. successful, error, redirected, etc)." or whatever
There was a problem hiding this comment.
displayValue... for the display values var names makes more sense to me instead of prefixing with an error, all the displayValues prefix with displayValue and group them together as such. IMO
There was a problem hiding this comment.
ok, maybe we could use a different word than "error" then?
There was a problem hiding this comment.
Changed this one, but I think we should keep displayValueValidationError because they literally are validation errors.
| /** Explanatory message stating that there was a failure in an audit caused by a missing page viewport meta tag configuration. */ | ||
| explanationViewport: 'Text is illegible because there\'s no viewport meta tag optimized ' + | ||
| 'for mobile screens.', | ||
| /** Explanatory message stating that there was a failure in an audit caused by a certain percentage of the text on the page being too small. */ |
There was a problem hiding this comment.
| /** Explanatory message stating that there was a failure in an audit caused by a certain percentage of the text on the page being too small. */ | |
| /** Explanatory message stating that there was a failure in an audit caused by a certain percentage of the text on the page being too small. "decimalProportion" will be replaced by a percentage between 0 and 100%. */ |
| 'for mobile screens.', | ||
| /** Explanatory message stating that there was a failure in an audit caused by a certain percentage of the text on the page being too small. */ | ||
| explanation: '{decimalProportion, number, extendedPercent} of text is too small.', | ||
| /** Explanatory message stating that there was a failure in an audit caused by a certain percentage of the text on the page being too small, based on a sample size of text that was less than 100% of the text on the page. */ |
There was a problem hiding this comment.
| /** Explanatory message stating that there was a failure in an audit caused by a certain percentage of the text on the page being too small, based on a sample size of text that was less than 100% of the text on the page. */ | |
| /** Explanatory message stating that there was a failure in an audit caused by a certain percentage of the text on the page being too small, based on a sample size of text that was less than 100% of the text on the page. "decimalProportion" and "decimalProportionVisited" will be replaced by percentages between 0 and 100%. */ |
| const UIStrings = { | ||
| /** Title of a Lighthouse audit that provides detail on the robots.txt file on the page. This descriptive title is shown when the robots.txt file is present and configured correctly. */ | ||
| title: 'robots.txt is valid', | ||
| /** Descriptive title of a Lighthouse audit that provides detail on the robots.txt file on the page. This imperative title is shown when the robots.txt file is misconfigured, which makes the page hard or impossible to scan via web crawler. */ |
There was a problem hiding this comment.
| /** Descriptive title of a Lighthouse audit that provides detail on the robots.txt file on the page. This imperative title is shown when the robots.txt file is misconfigured, which makes the page hard or impossible to scan via web crawler. */ | |
| /** Title of a Lighthouse audit that provides detail on the robots.txt file on the page. This descriptive title is shown when the robots.txt file is misconfigured, which makes the page hard or impossible to scan via web crawler. */ |
| const UIStrings = { | ||
| /** Title of a Lighthouse audit that provides detail on the site's robots.txt file. Note: "robots.txt" is a canonical filename and should not be translated. This descriptive title is shown when the robots.txt file is present and configured correctly. */ | ||
| title: 'robots.txt is valid', | ||
| /** Descriptive title of a Lighthouse audit that provides detail on the site's robots.txt file. Note: "robots.txt" is a canonical filename and should not be translated. This imperative title is shown when the robots.txt file is misconfigured, which makes the page hard or impossible to scan via web crawler. */ |
There was a problem hiding this comment.
| /** Descriptive title of a Lighthouse audit that provides detail on the site's robots.txt file. Note: "robots.txt" is a canonical filename and should not be translated. This imperative title is shown when the robots.txt file is misconfigured, which makes the page hard or impossible to scan via web crawler. */ | |
| /** Descriptive title of a Lighthouse audit that provides detail on the site's robots.txt file. Note: "robots.txt" is a canonical filename and should not be translated. This title is shown when the robots.txt file is misconfigured, which makes the page hard or impossible to scan via web crawler. */ |
| const i18n = require('../../../lib/i18n/i18n.js'); | ||
|
|
||
| const UIStrings = { | ||
| /** Description of a Lighthouse audit that provides detail on the structured data in a page. Structured data is standardized data on a page that helps a search engine categorize and understand its contents. This is displayed after a user expands the section to see more. No character length limits. 'Learn More' becomes link text to additional documentation. */ |
There was a problem hiding this comment.
Is "structured data" a specific enough term that we should ask that it should be left untranslated? I really have no idea.
We can always leave it to be translated and users can click on the links to learn more.
| explanationPointsElsewhere: 'Points to another hreflang location ({url})', | ||
| /** [ICU Syntax] Explanatory message stating that there was a failure in an audit caused by a URL pointing to a different domain. "url" will be replaced by the invalid URL (e.g. https://example.com). */ | ||
| explanationDifferentDomain: 'Points to a different domain ({url})', | ||
| /** Explanatory message stating that the page's canonical URL was pointing to a 'root of the same origin' which is a common mistake. The canonical URL should point a different page with the same content whose URL is preferred; however, in this case the canonical URL points to the root page (which is of the same origin) i.e. http://example.com/post1?alt=true -points to-> http://example.com/ when it should point to something like http://example.com/post1. In this context "root" is the starting page of a website, and "origin" is the domain the website is registered under. */ |
There was a problem hiding this comment.
ha, this is still a bit confusing :) :)
Maybe it would be easier to define "points", "root", and "origin" rather than giving the context?
|
@patrickhulce you still have a blocking review request if you want to take another look. |
Co-Authored-By: exterkamp <[email protected]>
Co-Authored-By: exterkamp <[email protected]>
Co-Authored-By: exterkamp <[email protected]>
…d tests with new UI strings.
| description: 'If your robots.txt file is malformed, crawlers may not be able to understand ' + | ||
| 'how you want your website to be crawled or indexed.', | ||
| /** [ICU Syntax] Label for the audit identifying that the robots.txt request has returned a specific HTTP status code. Note: "robots.txt" is a canonical filename and should not be translated. "statusCode" will be replaced with a 3 digit integer which represents the status of the HTTP connectiong for this page. */ | ||
| displayValueHttpBadCode: 'request for robots.txt returned HTTP status: {statusCode, number}', |
There was a problem hiding this comment.
displayValueHttpBadCode
<3 :)
|
🇦🇫 🇦🇽 🇦🇱 🇩🇿 🇦🇸 🇦🇩 🇦🇴 🇦🇮 🇦🇶 🇦🇬 🇦🇷 🇦🇲 🇦🇼 🇦🇨 🇦🇺 🇦🇹 🇦🇿 🇧🇸 🇧🇭 🇧🇩 🇧🇧 🇧🇾 🇧🇪 🇧🇿 🇧🇯 🇧🇲 🇧🇹 🇧🇴 🇧🇦 🇧🇼 🇧🇻 🇧🇷 🇮🇴 🇻🇬 🇧🇳 🇧🇬 🇧🇫 🇧🇮 🇰🇭 🇨🇲 🇨🇦 🇮🇨 🇨🇻 🇧🇶 🇰🇾 🇨🇫 🇪🇦 🇹🇩 🇨🇱 🇨🇳 🇨🇽 🇨🇵 🇨🇨 🇨🇴 🇰🇲 🇨🇬 🇨🇩 🇨🇰 🇨🇷 🇨🇮 🇭🇷 🇨🇺 🇨🇼 🇨🇾 🇨🇿 🇩🇰 🇩🇬 🇩🇯 🇩🇲 🇩🇴 🇪🇨 🇪🇬 🇸🇻 🇬🇶 🇪🇷 🇪🇪 🇪🇹 🇪🇺 🇫🇰 🇫🇴 🇫🇯 🇫🇮 🇫🇷 🇬🇫 🇵🇫 🇹🇫 🇬🇦 🇬🇲 🇬🇪 🇩🇪 🇬🇭 🇬🇮 🇬🇷 🇬🇱 🇬🇩 🇬🇵 🇬🇺 🇬🇹 🇬🇬 🇬🇳 🇬🇼 🇬🇾 🇭🇹 🇭🇲 🇭🇳 🇭🇰 🇭🇺 🇮🇸 🇮🇳 🇮🇩 🇮🇷 🇮🇶 🇮🇪 🇮🇲 🇮🇱 🇮🇹 🇯🇲 🇯🇵 🇯🇪 🇯🇴 🇰🇿 🇰🇪 🇰🇮 🇽🇰 🇰🇼 🇰🇬 🇱🇦 🇱🇻 🇱🇧 🇱🇸 🇱🇷 🇱🇾 🇱🇮 🇱🇹 🇱🇺 🇲🇴 🇲🇰 🇲🇬 🇲🇼 🇲🇾 🇲🇻 🇲🇱 🇲🇹 🇲🇭 🇲🇶 🇲🇷 🇲🇺 🇾🇹 🇲🇽 🇫🇲 🇲🇩 🇲🇨 🇲🇳 🇲🇪 🇲🇸 🇲🇦 🇲🇿 🇲🇲 🇳🇦 🇳🇷 🇳🇵 🇳🇱 🇳🇨 🇳🇿 🇳🇮 🇳🇪 🇳🇬 🇳🇺 🇳🇫 🇲🇵 🇰🇵 🇳🇴 🇴🇲 🇵🇰 🇵🇼 🇵🇸 🇵🇦 🇵🇬 🇵🇾 🇵🇪 🇵🇭 🇵🇳 🇵🇱 🇵🇹 🇵🇷 🇶🇦 🇷🇪 🇷🇴 🇷🇺 🇷🇼 🇼🇸 🇸🇲 🇸🇹 🇸🇦 🇸🇳 🇷🇸 🇸🇨 🇸🇱 🇸🇬 🇸🇽 🇸🇰 🇸🇮 🇸🇧 🇸🇴 🇿🇦 🇬🇸 🇰🇷 🇸🇸 🇪🇸 🇱🇰 🇧🇱 🇸🇭 🇰🇳 🇱🇨 🇲🇫 🇵🇲 🇻🇨 🇸🇩 🇸🇷 🇸🇯 🇸🇿 🇸🇪 🇨🇭 🇸🇾 🇹🇼 🇹🇯 🇹🇿 🇹🇭 🇹🇱 🇹🇬 🇹🇰 🇹🇴 🇹🇹 🇹🇦 🇹🇳 🇹🇷 🇹🇲 🇹🇨 🇹🇻 🇺🇬 🇺🇦 🇦🇪 🇬🇧 🏴 🏴 🏴 🇺🇸 🇺🇾 🇺🇲 🇻🇮 🇺🇿 🇻🇺 🇻🇦 🇻🇪 🇻🇳 🇼🇫 🇪🇭 🇾🇪 🇿🇲 🇿🇼 |
Summary
🇺🇸 🇬🇧 🇩🇪 🇮🇳 🇮🇨 🇲🇽
i18n for SEO category audits.
If you have opinions on the i18n description text please comment, I am not the best at explaining things in those also spelling is hard.