No i18n support: components should provide an object with Labels as props
janus-reith opened this issue · 23 comments
Some components render localized text, but provide no way to alter it outside of the component.
Example:
The component library should not be responsible for i18n itself.
It could accept an object with labels as props, in a similar manner compnents are passed and overrriden. The current strings could serve as fallback.
This is a known deficiency that will eventually have a solution. The workaround in the meantime is to override the entire component using the components context, providing a version of it that you've modified to use whatever i18n solution your app uses.
Yes, that would work in the meantime, thank you.
Would you accept PRs that would allow passing own strings(so still i18n-library-agnostic), or does this need further internal discussion?
@nnnnat Any thoughts here? Some components already accept label overrides (e.g., otherAddressLabel prop on AddressChoice). I think it makes sense to accept PRs that do this same thing for other components that have English text built into them. It's the simplest way to support translations without buying into any particular framework.
Can labels be added for this issue? I think its a fairly easy starting issue.
Hey, this issue doesn't seem to get a lot of traction. When it comes to components every single component that has text would need to get an extra prop for translation.
Is there any possibilty where we can slowly tackle this issue by doing components in batches?
Also, could I be assigned to this issue? We already did a big part when it comes to translations in our component library.
@HarisSpahija please feel free to create PRs to this repo that add label overrides for component text. We'd prefer 1 component per PR and not in batches just to keep reviews on our side shorter.
@nnnnat could you check if the current PR's are viable to be implemented?
@HarisSpahija I approved PRs. One needs a small change. Someone with ability to override the snyk checks will have to merge them, or you can merge master into them after the Snyk-related PRs have been merged.
Some components have strings that are possibly gramatically different in other languages. For example:
<StockWarning />
return <Span className={className}>Only {inventoryQuantity} in stock</Span>;In Dutch one would say "Slechts 7 op voorraad", while in Bosnian you could say: "7 Na Lageru". Grammar should be counted in when implementing i18next.
All components should have translation options now with a couple of exceptions. @janus-reith we could move to adding i18n to example-storefront now
@HarisSpahijaPon For those that need interpolation, you can update to take a fn prop instead.
// default props
getLowStockText: (data) => `Only ${data.inventoryQuantity} in stock`
const { getLowStockText } = this.props;
return <Span className={className}>{getLowStockText({ inventoryQuantity })}</Span>;
When using in translated app, you'd pass through to i18next in the function prop:
getLowStockText={(data) => i18next.t("lowStockText", data)}And your translation file would have "Only {{inventoryQuantity}} in stock" as the English value, for example.
See https://www.i18next.com/translation-function/interpolation
okay thanks! I will update them @aldeed
I think pretty much all components except the interpolation items have been set now. Is it time to implement a basic example to the storefront for i18n?
@janus-reith , would you be interested in helping with this issue? I am not sure what exactly is needed to make a sufficient example for users to test. Would be cool if we can implement it before 3.0 release.
@HarisSpahijaPon Great work, sure we can collaborate on this.
Hey guys @HarisSpahijaPon @janus-reith , I'm also interested in collaborating on this, please let me know how i can help.
@CristianCucunuba Id say check out this issue: reactioncommerce/example-storefront#524
You could fork the master branch and create an example using the new label props to show translations. I think its best if we start with one component and one language. Preferably English.
Also there are still some PR's open with some labels needing to be adjusted. Feel free to continue with my PR.
I noticed that some components are using other components from the component library. Components such as MiniCart make it so labels cannot be passed to MiniCartSummary without either overriding componentContext or passing props from the parent. For example:
<MiniCartSummary displaySubtotal={summary.itemTotal.displayAmount} />
Can be translated with the override labels
<MiniCartSummary displaySubtotal={summary.itemTotal.displayAmount} checkoutButtonText={"test"} footerMessageText={"test"}/>
Because the component is nested there is no way we can reach the override labels. This should be fixed in the component library or an extra props should be added to pass nested props.
What do you think would be the right approach to this @aldeed ?
@HarisSpahija I tend to prefer specific props like miniCartSummaryCheckoutButtonText, etc. which are passed down. This allows specific prop validation whereas a miniCartSummaryProps prop opens the door to hundreds of potential combinations that would need to be tested and supported.
That said, I don't know that Reaction has a specific policy on this. @mikemurray or @nnnnat may have more to say.
It's not quite correct to say there's no way to do it, though. If you use the components context to inject translations for MiniCartSummary (and all components), which would be the best and recommended way, then it will be translated wherever it is used without any need for passing down props. That's really the main reason why this package supports a component context: ability to override nested components.
I tried setting an override in component context, but I couldnt get the syntax to work without sideffects. Could you elaborate how to do this with an example?
@HarisSpahija I didn't test this, but it should work as long as you provide a component (e.g. a function that returns JSX).
{
MiniCartSummary: (props) => <MiniCartSummary checkoutButtonText={i18next.t("MiniCartSummary.checkoutButtonText")} footerMessageText={i18next.t("MiniCartSummary.footerMessageText")} {...props} />
}You'd have to make a slightly more complicated component that uses useTranslation hook to get a properly reactive version of i18next.t function, but this is the general idea.
Some components require some references or not every component is listed in the componentContext file, we should refactor those to make it easier for developers to override the components.
Agree that no components should directly import any other components unless there's clearly no use case for overriding it. Submitting individual GH issues for each case of that would be best.