-
Notifications
You must be signed in to change notification settings - Fork 894
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
Merged
Merged
Add Service Banners #5272
Changes from 1 commit
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
a83a5e6
Finish rendering
ammario 0a7a030
Begin form work
ammario ba450b3
Backend WIP
ammario afd8fb9
Finish backend
ammario d3a4485
Begin integrating color picker
ammario 290a31d
Pretty up form
ammario 84735e3
Automatic live preview
ammario 35b41e7
Reduce round trip on SET_BANNER
ammario c546602
Support unentitled flow
ammario e4255d9
Write storybook
ammario 96a5b70
Add docs
ammario 1ce7d08
docs: add Service Banners to enterprise.md
ammario e79916a
Remove old comment
ammario 335fdc4
Fix go tests
ammario 8b8cf0a
Merge remote-tracking branch 'origin/main' into service-banners
ammario 51809d7
Address review comments
ammario e5484d4
Use translations
ammario 2f46ad5
fixup! Use translations
ammario c28931e
Merge remote-tracking branch 'origin/main' into service-banners
ammario a49120f
make fmt
ammario File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Begin integrating color picker
- Loading branch information
commit d3a4485a57e73949072d04d196f81a64414d8bdf
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 usereact-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.
Makes sense
My understanding was that we began using Language objects to aid in testing, with translations as a secondary goal?
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!