Rewrite pluralize to be more generic and future proof for localized translation of UI elements
#1
Conversation
dobromir-hristov
left a comment
dobromir-hristov
left a comment
There was a problem hiding this comment.
This looks great to me. I wonder a few things:
- Should we extract such text strings to language files, so its easier to maintain and reduce clutter in the vue files?
- Should we allow defining just
other, in cases where the word does not have aonevariant?
| // at minimum, there should at least be a "one" and "other" choice for the | ||
| // "en" locale for use as fallback text in the case that a choice for the | ||
| // user's resolved locale category is not provided/available | ||
| if (!choices[defaultLocale] || !choices[defaultLocale][defaultCategory]) { |
There was a problem hiding this comment.
Should we allow for just specifying other, so we dont duplicate one and other, in cases like the Extends?
There was a problem hiding this comment.
I'm not sure to be honest. My initial thought is no, because the pluralize function isn't smart enough to know that we want to use the same word for both cases in that specific scenario. I think it should have the expectation that each language provided in the choices argument should have text to use for every plural category that the specific language may need. We could hardcode in special logic for this scenario in English maybe, but I think I would prefer to avoid that—maybe we could have a helper function for generating the duplicate entries if this ends up being a common enough problem?
It felt a little weird to even have this hardcoded logic checking for all the English options, but I think that makes sense for our app since we'll want some default language to show in every place where this function is used, even if we don't have a particular translation for some other language (in the future).
There was a problem hiding this comment.
Also, thinking about it a bit more—it's more of a weird thing we're doing in the code for this situation since "Extends" isn't really a noun that we want to potentially pluralize like we do for Framework/Technology, so potentially it would make more sense to provide the ability to pass a static title that doesn't get pluralized for that type of situation. I think it's fine as-is for now though.
Definitely in the future. I think once we start working on localization in the future, it would totally make sense to create some abstraction for having constants that refer to separate files where translations/pluralities are stored separately in JSON or strings files or whatever. I'm not sure we need it at the moment since we're only doing this for one piece of data. |
| v-if="extendsTechnology" | ||
| class="extends-technology" | ||
| title="Extends" | ||
| :title="{ en: { one: 'Extends', other: 'Extends' } }" |
There was a problem hiding this comment.
This line was really confusing to me at first. I second what Marcus said above about providing the ability to pass a static title that doesn't get pluralized. Maybe that means creating another Plural category? Something like 'noChange', or 'verb'? Or now that I think about it, technically this case is pluralizing 'extend'. So could we just call the pluralize function with pluralize('extend', 2)?
There was a problem hiding this comment.
This line was really confusing to me at first. I second what Marcus said above about providing the ability to pass a static title that doesn't get pluralized.
Makes sense. Really we shouldn't even try to pluralize "Extends" in this scenario since it isn't even a noun. I made that change with 311de9e
Maybe that means creating another Plural category? Something like 'noChange', or 'verb'? Or now that I think about it, technically this case is pluralizing 'extend'. So could we just call the pluralize function with pluralize('extend', 2)?
Agree that we shouldn't try to pluralize that word specifically. I don't agree that we should workaround this type of situation with a new plural category that doesn't fit into the normal standard.
3 participants
I re-wrote the
pluralizefunction to take an object that has translated text for each plural category of given languages and removed the hardcoded rules for appending text to existing words that only works for certain words in English. Although we only support English today, this allows us to more easily bring in alternate translations when we have them (and removes the existing logic that would be difficult to maintain as our UI vocabulary grows). All of the existing use cases ofpluralizewere fixed to use this new API.