-
Notifications
You must be signed in to change notification settings - Fork 887
Add Service Banners #5272
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Service Banners #5272
Conversation
site/src/pages/DeploySettingsPage/ServiceBannerSettingsPage.tsx
Outdated
Show resolved
Hide resolved
site/src/pages/DeploySettingsPage/ServiceBannerSettingsPage.tsx
Outdated
Show resolved
Hide resolved
backgroundColorLabel: "Background Color", | ||
updateBanner: "Update", | ||
previewBanner: "Preview", | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're no longer using the Language
convention because it isn't conducive to translation and also gets stale without warning. Do you mind updating this PR to use react-i18next
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. What do you by "gets stale without warning"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The translation aspect I've never understood, because it's unlikely we'd translate the product for years.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ammario, the Language objects are just POJOs so no warning is thrown if we cease to use a string in our JSX. Thus you can have key-value pairs that aren't used, and this was happening more frequently as we grew the code base and refactored.
Regarding translations in general: I don't think we need translations, but I didn't introduce them; we had them when I arrived in the form of these Language objects. I just figured if we were going to bother with translating at all, we might as well use a very popular and robust solution versus something home-baked that wasn't maintainable.
There are a couple of other reasons we might want to use i18next: it's really nice to have strings all colocated in one area so that we can make sure the verbiage we use is consistent. Libraries like i18next come with features that handle quirks like pluralization and interpolation - thus we don't have to have giant if-blocks in our JSX. Lastly, although the day may be far in the future, if we ever want to translate it's nice to start now when our code base is relatively small and simple.
My two cents! Happy to discuss further online if you'd like. Also happy to consider just putting strings directly in the JSX. I'd just rather not use the Language objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ammario, the Language objects are just POJOs so no warning is thrown if we cease to use a string in our JSX. Thus you can have key-value pairs that aren't used, and this was happening more frequently as we grew the code base and refactored.
Makes sense
Regarding translations in general: I don't think we need translations, but I didn't introduce them; we had them when I arrived in the form of these Language objects. I just figured if we were going to bother with translating at all, we might as well use a very popular and robust solution versus something home-baked that wasn't maintainable.
My understanding was that we began using Language objects to aid in testing, with translations as a secondary goal?
There are a couple of other reasons we might want to use i18next: it's really nice to have strings all colocated in one area so that we can make sure the verbiage we use is consistent. Libraries like i18next come with features that handle quirks like pluralization and interpolation - thus we don't have to have giant if-blocks in our JSX. Lastly, although the day may be far in the future, if we ever want to translate it's nice to start now when our code base is relatively small and simple.
My two cents! Happy to discuss further online if you'd like. Also happy to consider just putting strings directly in the JSX. I'd just rather not use the Language objects.
Thank you for sharing, I see those benefits. I struggle with the react-i18next
developer experience. Working with JSON is not fun, not conducive to auto-complete, and lacks type errors. There are also many places where we want to intersperse a link or other HTML element into the language, and I don't see a clean way of doing that.
In this PR I have transitioned some of the strings to use translations to be consistent with the rest of the codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ammario I was told the Language blocks were a way to get started on translations without committing to a library. They're actually kind of annoying to use in tests as if you pull in different components, you have to alias each Language block separately, but maybe there's a benefit I'm overlooking.
To add HTML or links, you'll want to use the library's Trans component. Mathias recently merged up a great example, here. I'll always agree that i18n is more of a hassle than straight JSX. If it feels particularly painful, we can certainly take it out, but should probably discuss in FE variety first; feel free to throw something on the agenda if you'd like!
TODO:
Resolves #4322