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

misc(treemap): i18n #12441

Merged
merged 20 commits into from
May 14, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 30 additions & 1 deletion build/build-treemap.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,33 @@
const fs = require('fs');
const GhPagesApp = require('./gh-pages-app.js');

/**
* Extract only the strings needed for lighthouse-treemap into
* a script that sets a global variable `strings`, whose keys
* are locale codes (en-US, es, etc.) and values are localized UIStrings.
*/
function buildStrings() {
const locales = require('../lighthouse-core/lib/i18n/locales.js');
const UIStrings = require('../lighthouse-treemap/app/src/util.js').UIStrings;
const strings = /** @type {import('../lighthouse-treemap/types/treemap').Strings} */ ({});

for (const [locale, lhlMessages] of Object.entries(locales)) {
const localizedStrings = Object.fromEntries(
Object.entries(lhlMessages).map(([icuMessageId, v]) => {
const [filename, varName] = icuMessageId.split(' | ');
if (!filename.endsWith('util.js') || !(varName in UIStrings)) {
return [];
}

return [varName, v];
})
);
strings[/** @type {LH.Locale} */ (locale)] = localizedStrings;
}

return 'const strings =' + JSON.stringify(strings, null, 2) + ';';
}

/**
* Build treemap app, optionally deploying to gh-pages if `--deploy` flag was set.
*/
Expand All @@ -28,7 +55,9 @@ async function run() {
fs.readFileSync(require.resolve('tabulator-tables/dist/js/modules/format.js'), 'utf8'),
fs.readFileSync(require.resolve('tabulator-tables/dist/js/modules/resize_columns.js'), 'utf8'),
/* eslint-enable max-len */
{path: 'src/*'},
buildStrings(),
{path: '../../lighthouse-core/report/html/renderer/i18n.js'},
{path: 'src/**/*'},
],
assets: [
{path: 'debug.json'},
Expand Down
2 changes: 1 addition & 1 deletion build/gh-pages-app.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ const license = `/*
* @return {string[]}
*/
function loadFiles(pattern) {
const filePaths = glob.sync(pattern);
const filePaths = glob.sync(pattern, {nodir: true});
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no longer necessary, but is more correct.

return filePaths.map(path => fs.readFileSync(path, {encoding: 'utf8'}));
}

Expand Down
3 changes: 2 additions & 1 deletion lighthouse-core/lib/i18n/locales.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@
*/
'use strict';

/** @fileoverview
/**
* @fileoverview
* Define message file to be used for a given locale. A few aliases are defined below.
*
* Google locale inheritance rules: https://goto.google.com/ccssm
Expand Down
24 changes: 24 additions & 0 deletions lighthouse-core/lib/i18n/locales/en-US.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

24 changes: 24 additions & 0 deletions lighthouse-core/lib/i18n/locales/en-XL.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

41 changes: 39 additions & 2 deletions lighthouse-core/report/html/renderer/i18n.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,19 +9,25 @@

// Not named `NBSP` because that creates a duplicate identifier (util.js).
const NBSP2 = '\xa0';
const KiB = 1024;
const MiB = KiB * KiB;

/**
* @template T
Copy link
Member

Choose a reason for hiding this comment

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

parameterizing on strings when none of the I18n object uses strings except the getter for strings works for this PR but makes me think it really shouldn't be in this object at all and should just be a global (or on Util directly), but we can save that for a future report refactor PR :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moving to Util/TreemapUtil SGTM, and so does worrying about it in a future refactor ;)

*/
class I18n {
/**
* @param {LH.Locale} locale
* @param {LH.I18NRendererStrings=} strings
* @param {T=} strings
*/
constructor(locale, strings) {
// When testing, use a locale with more exciting numeric formatting.
if (locale === 'en-XA') locale = 'de';

this._numberDateLocale = locale;
this._numberFormatter = new Intl.NumberFormat(locale);
this._strings = /** @type {LH.I18NRendererStrings} */ (strings || {});
this._percentFormatter = new Intl.NumberFormat(locale, {style: 'percent'});
this._strings = /** @type {T} */ (strings || {});
}

get strings() {
Expand All @@ -39,6 +45,15 @@ class I18n {
return this._numberFormatter.format(coarseValue);
}

/**
* Format percent.
* @param {number} number 0–1
* @return {string}
*/
formatPercent(number) {
return this._percentFormatter.format(number);
}

/**
* @param {number} size
* @param {number=} granularity Controls how coarse the displayed value is, defaults to 0.1
Expand All @@ -50,6 +65,17 @@ class I18n {
return `${kbs}${NBSP2}KiB`;
}

/**
* @param {number} size
* @param {number=} granularity Controls how coarse the displayed value is, defaults to 0.1
* @return {string}
*/
formatBytesToMiB(size, granularity = 0.1) {
const formatter = this._byteFormatterForGranularity(granularity);
const kbs = formatter.format(Math.round(size / 1024 ** 2 / granularity) * granularity);
return `${kbs}${NBSP2}MiB`;
}

/**
* @param {number} size
* @param {number=} granularity Controls how coarse the displayed value is, defaults to 1
Expand All @@ -61,6 +87,17 @@ class I18n {
return `${kbs}${NBSP2}bytes`;
}

/**
* @param {number} size
* @param {number=} granularity Controls how coarse the displayed value is, defaults to 1
* @return {string}
*/
formatBytesWithBestUnit(size, granularity = 1) {
if (size >= MiB) return this.formatBytesToMiB(size, granularity);
if (size >= KiB) return this.formatBytesToKiB(size, granularity);
return this.formatNumber(size, granularity) + '\xa0B';
}

/**
* Format bytes with a constant number of fractional digits, i.e for a granularity of 0.1, 10 becomes '10.0'
* @param {number} granularity Controls how coarse the displayed value is
Expand Down
4 changes: 2 additions & 2 deletions lighthouse-core/report/html/renderer/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

/* globals self */

/** @typedef {import('./i18n')} I18n */
/** @template T @typedef {import('./i18n')<T>} I18n */

const ELLIPSIS = '\u2026';
const NBSP = '\xa0';
Expand Down Expand Up @@ -508,7 +508,7 @@ Util.getUniqueSuffix = (() => {
};
})();

/** @type {I18n} */
/** @type {I18n<typeof Util['UIStrings']>} */
// @ts-expect-error: Is set in report renderer.
Util.i18n = null;

Expand Down
5 changes: 2 additions & 3 deletions lighthouse-core/scripts/i18n/bake-ctc-to-lhl.js
Original file line number Diff line number Diff line change
Expand Up @@ -118,10 +118,9 @@ function saveLhlStrings(path, localeStrings) {

/**
* @param {string} dir
* @param {string} outputDir
* @return {Array<string>}
*/
function collectAndBakeCtcStrings(dir, outputDir) {
function collectAndBakeCtcStrings(dir) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

minor refactor, didn't need two parameters.

const lhlFilenames = [];
for (const filename of fs.readdirSync(dir)) {
const fullPath = path.join(dir, filename);
Expand All @@ -131,7 +130,7 @@ function collectAndBakeCtcStrings(dir, outputDir) {
if (!process.env.CI) console.log('Baking', relativePath);
const ctcStrings = loadCtcStrings(relativePath);
const strings = bakePlaceholders(ctcStrings);
const outputFile = outputDir + path.basename(filename).replace('.ctc', '');
const outputFile = path.join(dir, path.basename(filename).replace('.ctc', ''));
saveLhlStrings(outputFile, strings);
lhlFilenames.push(path.basename(filename));
}
Expand Down
9 changes: 6 additions & 3 deletions lighthouse-core/scripts/i18n/collect-strings.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ const UISTRINGS_REGEX = /UIStrings = .*?\};\n/s;

const foldersWithStrings = [
`${LH_ROOT}/lighthouse-core`,
`${LH_ROOT}/lighthouse-treemap`,
path.dirname(require.resolve('lighthouse-stack-packs')) + '/packs',
];

Expand All @@ -39,6 +40,7 @@ const ignoredPathComponents = [
'**/test/**',
'**/*-test.js',
'**/*-renderer.js',
'lighthouse-treemap/app/src/main.js',
];

/**
Expand Down Expand Up @@ -586,7 +588,9 @@ function collectAllStringsInDir(dir) {
if (seenStrings.has(ctc.message)) {
ctc.meaning = ctc.description;
const seenId = seenStrings.get(ctc.message);
if (seenId) {
// TODO: `strings[seenId]` check shouldn't be necessary here ...
// see https://github.com/GoogleChrome/lighthouse/pull/12441/files#r630521367
if (seenId && strings[seenId]) {
if (!strings[seenId].meaning) {
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
strings[seenId].meaning = strings[seenId].description;
collisions++;
Expand Down Expand Up @@ -646,8 +650,7 @@ if (require.main === module) {
console.log('Written to disk!', 'en-XL.ctc.json');

// Bake the ctc en-US and en-XL files into en-US and en-XL LHL format
const lhl = collectAndBakeCtcStrings(path.join(LH_ROOT, 'lighthouse-core/lib/i18n/locales/'),
path.join(LH_ROOT, 'lighthouse-core/lib/i18n/locales/'));
const lhl = collectAndBakeCtcStrings(path.join(LH_ROOT, 'lighthouse-core/lib/i18n/locales/'));
lhl.forEach(function(locale) {
console.log(`Baked ${locale} into LHL format.`);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -569,7 +569,7 @@ describe('ReportUIFeatures', () => {
const container = render(sampleResults);
for (const node of dom.findAll('[data-i18n]', container)) {
const val = node.getAttribute('data-i18n');
assert.ok(val in Util.UIStrings, `Invalid data-i18n value of: "${val}" found.`);
assert.ok(val in Util.UIStrings, `Invalid data-i18n value of: "${val}" not found.`);
}
});
});
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-treemap/app/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@

<span class="lh-header__inputs">
<select class="bundle-selector"></select>
<button class="lh-button lh-button--active lh-button--toggle-table">Toggle Table</button>
<button data-i18n="toggleTableButtonLabel" class="lh-button lh-button--active lh-button--toggle-table"></button>
</span>
</div>

Expand Down