Conversation
- `tabs.js` enriches `tabs.json` with l10n IDs generated from the text. - `l10n.js` derives a strings Map for the extractor
|
c2d788d was deployed to: https://fred-pr1403.review.mdn.allizom.net/ |
LeoMcA
left a comment
LeoMcA
left a comment
There was a problem hiding this comment.
Only done an initial skim so far, but nice idea. I imagine you've already thought of this, but this approach of exposing strings at "l10n extract" runtime would allow us to localise strings coming from an upstream source as well - nice!
Let's add some simple tests, with some simple fixtures, in the same vein as the current ones.
| entries.push([tab.buttonL10nId.long, tab.buttonText.long], [tab.buttonL10nId.short, tab.buttonText.short]); | ||
| } | ||
|
|
||
| if (!("panelTitle" in tab)) return entries; |
There was a problem hiding this comment.
nit: I'd prefer an if("panelTitle" in tab) { block wrapping the below lines, than a negated condition - makes the code read easier IMO
| entries.push([group.titleL10nId, /** @type {string} */ (group.title)]); | ||
| } | ||
| for (const item of group.items) { | ||
| if (!("l10nId" in item)) continue; |
There was a problem hiding this comment.
nit: similarly, a non-negated if would read better imo
| const { strings } = await import( | ||
| pathToFileURL(path.join(componentsDir, file)).href | ||
| ); | ||
| if (!(strings instanceof Map)) continue; |
There was a problem hiding this comment.
nit: avoid the negated if, wrap the section below
| for (const [id, value] of strings) { | ||
| if (map.has(id) && map.get(id) !== value) { | ||
| throw new Error( | ||
| `L10n extractor: \`${id}\` is a duplicate id with different text`, |
| for (const [id, value] of await scrapeL10nModules(componentsDir)) { | ||
| if (tags.has(id) && tags.get(id) !== value) { | ||
| throw new Error( | ||
| `L10n extractor: \`${id}\` is a duplicate id with different text`, |
| href=${tab.href} | ||
| data-glean-id=${`menu_click_link: top-level -> ${tab.href}`} | ||
| >${tab.buttonText}</a | ||
| >${context.l10n(tab.buttonL10nId)}</a |
There was a problem hiding this comment.
The plain context.l10n function doesn't work like this any more, perhaps that's an oversight from me.
You have to use context.l10n.raw({ id: ... if you're not providing the en-US text inline.
| const slugify = (str) => | ||
| str | ||
| .toLowerCase() | ||
| .replaceAll(/[^\p{L}\p{N}]+/gu, "-") |
There was a problem hiding this comment.
I'm not sure if fluent ids support non-ascii characters, perhaps simplify to [^a-z0-9]?
| menu-tools-0-playground = Playground | ||
| menu-tools-0-http-observatory = HTTP Observatory | ||
| menu-tools-1-border-image-generator = Border-image generator | ||
| menu-tools-1-border-radius-generator = Border-radius generator | ||
| menu-tools-1-box-shadow-generator = Box-shadow generator | ||
| menu-tools-1-color-format-converter = Color format converter | ||
| menu-tools-1-color-mixer = Color mixer | ||
| menu-tools-1-shape-generator = Shape generator |
There was a problem hiding this comment.
I don't think the number is necessary here, and just means we'll have unstable ids if we rearrange things
| * Imports all `l10n.js` modules under the components directory and merges | ||
| * their exported `strings` Maps into a single Map. | ||
| * | ||
| * @param {string} componentsDir |
There was a problem hiding this comment.
Don't think it's necessary to pass componentsDir as a param
| user-menu-user = User | ||
| writer-open-editor-open-in-editor = Open in editor | ||
| writer-toolbar-view-on-mdn = View on MDN | ||
| menu-html = HTML |
There was a problem hiding this comment.
As these strings don't exist in en-US.ftl, we don't actually have anything to render:
I'd suggest that the extraction logic you've added should add them to en-US.ftl, then allow the usual en-US.ftl -> template.ftl logic to copy them here.
3 participants
Description
Makes the menu entries localizable.
Motivation
Ensure other locales don't have to live with the English menu.
Additional details
Related issues and pull requests
Fixes #1402.