feat: add localization for meta endpoints #288
Conversation
src/interfaces/strategy.ts
Outdated
| @@ -151,7 +151,8 @@ export class StrategyInterface<T extends ChainId> extends ServiceInterface<T> { | |||
| const metadata: StrategyMetadata = { | |||
| address: strategy.address, | |||
| name: metadatum?.name || strategy.name || "Strategy", | |||
| description: metadatum?.description ?? "I don't have a description for this strategy yet", | |||
| description: | |||
| metadatum?.localization[this.ctx.locale]?.description ?? "I don't have a description for this strategy yet", | |||
There was a problem hiding this comment.
fallback to the english description first instead of "I don't have a description for this strategy yet"? Since that's in english anyway
There was a problem hiding this comment.
Agreed if we are certain that English is always available.
Otherwise, I'd say "No description available" or something more generic.
There was a problem hiding this comment.
metadatum.description should always be available and in english, so I'm confident we can fallback to that
src/utils/localization.ts
Outdated
| property: T; | ||
| locale: Locale; | ||
| fallback: string; | ||
| }): string => obj?.localization[locale]?.description ?? (obj && obj[property]) ?? fallback; |
There was a problem hiding this comment.
it's a shame that we've lost the fallback to english here first, not sure how messy that would make the code to add it back, but I think it would be better from a product perspective
There was a problem hiding this comment.
I mean now it's part of whoever wants to localize a string to set a fallback for it, which in these cases would be the original english string, and fallback is always required so it is needed to provide that string
There was a problem hiding this comment.
We could add it here, but we'd need a fallback object with all the fallbacks, and then if a fallback is not provided we default to English. I think that makes sense in order to have it centralised.
There was a problem hiding this comment.
Isnt the english text defaulted in obj && property && obj[property] ? From what I see in yearn-meta, that is how it has been structured, and should always exist.
There was a problem hiding this comment.
Or you mean something like:
obj?.localization[locale]?.description ?? obj?.localization['en']?.description ?? (obj && property && obj[property]) ?? fallback
?
There was a problem hiding this comment.
I think what @jstashh was referring to is that we shouldn't need a required fallback prop and it would just fallback to English in the SDK. If the fallback prop is given then it would override that default fallback.
There was a problem hiding this comment.
obj?.localization[locale]?.description ?? obj?.localization['en']?.description ?? (obj && property && obj[property]) ?? fallback
@xgambitox yes something like that, since we know english will always be there and that it would probably be more useful than a generic error message (I don't have a description for this strategy yet)
There was a problem hiding this comment.
not a blocker though - let's merge this to move forwards
There was a problem hiding this comment.
ok, will merge then, and if it becomes an issue we will revisit it
There was a problem hiding this comment.
LGTM, I do agree with @jstashh that we could have a defaultFallback if none is provided.
* feat: add localization for meta endpoints * test: add tests for token localization * fix: fix styling * feat: pr feedback * fix: pr feedback
Description
Adds localization to the sdk, accepting
Related Issue
Motivation and Context
We want to show localized strings for strategies and tokens data on any provided rfrom the sdk
How Has This Been Tested?
Manually and some unit tests
Screenshots (if appropriate):
(Screenshot for initializing sdk with 'es' locale while main application is in 'en' locale)
