feat(prerender): Use real strings and text direction for per-locale prerendered html #3740
Conversation
Mardak
force-pushed
the
gh3370-strings
branch
3 times, most recently
from
c0cda59
to
6e8683c
Compare
bin/render-activity-stream-html.js
Outdated
| @@ -1,18 +1,176 @@ | |||
| /* eslint-disable no-sync */ | |||
| /* eslint-disable no-console, no-sync */ | |||
There was a problem hiding this comment.
I know this is unrelated but wonder if we should actually get rid of the no-sync rule in general, since we're only using node for build scripts and stuff
bin/render-activity-stream-html.js
Outdated
| }; | ||
|
|
||
| // From https://hg.mozilla.org/l10n-central/ | ||
| // console.log(`const MOZILLA_CENTRAL_LOCALES = ${JSON.stringify([...document.querySelectorAll("a.list:not([href*='x-testing']")].map(a => a.textContent.trim()), null, 2)};`) | ||
| const MOZILLA_CENTRAL_LOCALES = [ |
There was a problem hiding this comment.
Do we have a plan to keep this in sync/write a test or something? Or is this something that almost never change(d/s)?
There was a problem hiding this comment.
So… "ia" was added since I originally created a local branch 1 month ago, and it has 100% strings localized for activity stream: https://pontoon.mozilla.org/ia/activity-stream-new-tab/strings.properties/
Would be unfortunate if we didn't notice and missed the prerendering for that locale. But on that note, if we have strings for a locale that we aren't building, we should be able to notice right away !
Should it break this render script? Big warning? I suppose the strings import script could be smarter, but then we'll want to share this array outside of this file.
Or alternatively, we can dynamically augment MOZILLA_CENTRAL_LOCALES with the locales that we have localized strings.
Actually, I think I'll go with the dynamic augment and big warning.
And just to be explicit, we can't rely on our own locale list from locales.json as we would have missed out on bn-IN, ja-JP-mac, ta-LK which we have similar strings to reuse from bn-BD, ja, ta respectively.
There was a problem hiding this comment.
Added a comment documenting the purpose of this list.
| const jarLines = fs.readFileSync(jarFile, "utf8").split("\n"); | ||
| const startLine = jarLines.findIndex(line => line.includes(scriptName)); | ||
| const endLine = jarLines.indexOf("#endif", startLine); | ||
| jarLines.splice(startLine, endLine - startLine + 1, ...[ |
bin/render-activity-stream-html.js
Outdated
| for (const [file, templater] of localizedFiles) { | ||
| fs.writeFileSync(path.join(basePath, file), templater({html, options, state})); | ||
| } | ||
| console.log(`Done writing "${locale}" localized files to ${basePath}.`); |
There was a problem hiding this comment.
Personally, I would do something like this for the output:
Writing localized files to /whatever/system-addon/prerendered
✔ en-US
✔ debug
✔ ach
✔ ar
...
Skipped the following locales because they had the same strings as en-US: af, ak, an, as, ...
Done.
You could maybe try something with process.stdout.write to get the locales on the same line, but yeah this obviously up to you
There was a problem hiding this comment.
Maybe.. I.. got.. carried.. away…
|
|
||
| console.log(`Done writing html and js to ${BASE_FILE_PATH}.`); // eslint-disable-line no-console | ||
| // Replace existing render-generated lines in jar.mn | ||
| const scriptName = path.basename(__filename); |
There was a problem hiding this comment.
Can you actually include a complete path here if someone is trying to find this when looking at mc?
There was a problem hiding this comment.
What's a "complete" path? "bin/render-activity-stream-html.js" or "https://github.com/mozilla/activity-stream/blob/master/bin/render-activity-stream-html.js"
There was a problem hiding this comment.
Used full url:
# These defines and ifs below are generated by https://github.com/mozilla/activity-stream/blob/master/bin/render-activity-stream-html.js
Mardak
force-pushed
the
gh3370-strings
branch
from
6e8683c
to
7b96719
Compare
Fix #3370. r?@k88hudson Running tests results in spewing these errors:
ERROR: '[React Intl] Missing message: "search_web_placeholder" for locale: "en-US"'Any good way to suppress? Or I suppose I could pass in actual strings…
Here's a screenshot of prerendered "ar":
As well as partially-"prerendered" (just the title/locale/dir) "ar":
