i18n: fix custom formatted ICU within plurals #9460
Conversation
exterkamp
requested review from
brendankenny,
patrickhulce and
paulirish
as code owners
patrickhulce
left a comment
patrickhulce
left a comment
There was a problem hiding this comment.
thanks for cleaning this up for realz @exterkamp! :D
lighthouse-core/lib/i18n/i18n.js
Outdated
| * Note: This function will recursively inspect plural elements for nested arguementElements. | ||
| * | ||
| * @param {Array<import('intl-messageformat-parser').Element>} parsedIcu | ||
| * @param {Map<string, import('intl-messageformat-parser').Element>} elements |
There was a problem hiding this comment.
unexpected that this returned a map, can we add the return type explicitly?
Co-Authored-By: Patrick Hulce <[email protected]>
lighthouse-core/lib/i18n/i18n.js
Outdated
| * | ||
| * Note: This function will recursively inspect plural elements for nested arguementElements. | ||
| * | ||
| * @param {Array<import('intl-messageformat-parser').Element>} elementsList |
There was a problem hiding this comment.
typically we just use plurals for arrays of things unless the list gives extra info about the value. I like @patrickhulce's earlier suggestion of icuElements (or messageElements...plain old elements is confusing in web-related terminology :) for this, personally.
brendankenny
left a comment
brendankenny
left a comment
There was a problem hiding this comment.
some last naming things, but otherwise LGTM!
what do you think about a follow up PR to add a check in collect-strings that there aren't strings that will run into the same issue as the one in resource-summary so we don't have to wait for translation feedback that we messed up again? Not worth it or easy to check?
(we maybe also want to add guidance to the readme on how things should be nested)
|
@patrickhulce over to you |
| * @param {Array<import('intl-messageformat-parser').Element>} icuElements | ||
| * @param {Map<string, import('intl-messageformat-parser').Element>} seenElementsById | ||
| */ | ||
| function _collectAllCustomElementsFromICU(icuElements, seenElementsById = new Map()) { |
There was a problem hiding this comment.
what an awesome journey this function as had...
it's so. good. now :D
4 participants
Summary
Allow custom formatted ICU
{var, number, bytes}to be nested inside{var, plural, ...}ICU blocks, and have it be formatted properlyRelated Issues/PRs
fixes: #9378