-
Notifications
You must be signed in to change notification settings - Fork 208
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
ui: Rename streams to channels in user-facing strings #707
Conversation
f12f305
to
b407423
Compare
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.
Thanks, This looks good to me :)
Updating l10n
data labels & template variables can be done later, as mentioned in #630 (comment)
Thanks @rajveermalviya for the first review! When advancing a review to the next stage, let's be sure to ping on the thread the next reviewer, as well as update the labels, to help make sure everyone's on the same page. So @kenclary over to you 🙂 (And please also take a look in your inbox for the invite I've sent for this repo; that'll let you update labels and reviewers in turn, but as a side effect it'll also make it possible to name you in the GitHub "request review" feature, because it'll make GitHub aware that you're connected to this org/repo.) |
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.
I love a straightforward PR.
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.
Thanks @Khader-1! Some small comments on the commit sequence:
Here are the commits in this revision:
3437549 intl: Use channel instead of stream
1227390 ui: Rename stream to channel in user-facing strings
b407423 notif: Rename stream to channel in displayed notifications
The first and second commits are both about changing strings that appear in the in-app UI, which at this point means all the UI except notifications. So I think it would be fine to squash them into one commit, with a summary line like:
ui: Rename stream to channel in user-facing strings, except notifications
(If you don't want to squash them together, then each commit should have some more words about exactly which strings are being treated. Basically the second commit is about the in-app strings that are still TODOs for #277.)
I think it would also be fine to squash the notifications-strings commit into that as well (i.e., squashing your revision's three commits into one), and have the commit message say that all user-facing strings are treated in the commit. That's still in line with our convention that a commit represents one coherent idea.
b407423
to
98098ba
Compare
Thanks @chrisbobbe! A new revision with a single commit PTAL. |
Great, LGTM! Marking for Greg's review now. |
Thanks all! Looks good — merging. I'll add a second commit on top, to update the Japanese translation. 🙂 (That's a small partial translation, which we have just for the sake of end-to-end testing that our i18n setup within the app works, pending #276 which will let us take translation contributions broadly like we do for other Zulip codebases.) FTR, the searches I did to confirm the rename was complete were:
Most of the results were in Looking forward to seeing most of those — and matches for |
Before launch, in order to manage contributions from many people for translations into many languages, we'll wire things up to an appropriate platform: zulip#276. For now, though, we have just this one partial translation into Japanese, as an end-to-end demo of the i18n setup within the app, and we maintain it directly in the repo. Update that translation to reflect this rename.
98098ba
to
e10ed0f
Compare
Fixes: #630