content: Add localized rendering of system-group names in @-mentions#2175
content: Add localized rendering of system-group names in @-mentions#2175gnprice merged 1 commit intozulip:mainfrom
Conversation
chrisbobbe
left a comment
chrisbobbe
left a comment
There was a problem hiding this comment.
Thanks! Small comments below.
| /// Get the display name to use in the UI for this system group. | ||
| String displayName(ZulipLocalizations zulipLocalizations) => switch (this) { | ||
| SystemGroupName.everyoneOnInternet => | ||
| zulipLocalizations.systemGroupNameEveryoneOnInternet, | ||
| SystemGroupName.everyone => | ||
| zulipLocalizations.systemGroupNameEveryoneIncludingGuests, | ||
| SystemGroupName.members => | ||
| zulipLocalizations.systemGroupNameEveryoneExceptGuests, | ||
| SystemGroupName.fullMembers => | ||
| zulipLocalizations.systemGroupNameFullMembers, | ||
| SystemGroupName.moderators => | ||
| zulipLocalizations.systemGroupNameModerators, | ||
| SystemGroupName.administrators => | ||
| zulipLocalizations.systemGroupNameAdministrators, | ||
| SystemGroupName.owners => | ||
| zulipLocalizations.systemGroupNameOwners, | ||
| SystemGroupName.nobody => | ||
| zulipLocalizations.systemGroupNameNobody, | ||
| }; |
There was a problem hiding this comment.
Why are these particular display names chosen? Is there API documentation you can link to in the dartdoc, so it's clear for people reading this code later?
There was a problem hiding this comment.
The API docs contain a general description/explanation of the system roles, but do not contain the particular strings used in the web app code. I will add a dartdoc comment saying that the strings are taken from the web app code.
| if (userGroup case UserGroup(:final name)) { | ||
| // TODO(#1260) Get display name for system groups using localization | ||
| nodes = [TextNode(node.isSilent ? name : '@$name')]; | ||
| final displayName = SystemGroupName.fromJson(name) | ||
| ?.displayName(ZulipLocalizations.of(context)) ?? name; | ||
| nodes = [TextNode(node.isSilent ? displayName : '@$displayName')]; | ||
| } |
There was a problem hiding this comment.
Let's do an explicit check on isSystemGroup, for example like this:
if (userGroup case UserGroup(:final name, :final isSystemGroup)) {
final String displayName;
if (isSystemGroup) {
final groupName = SystemGroupName.fromJson(name); // TODO(log) if null
displayName = groupName?.displayName(zulipLocalizations) ?? name;
} else {
displayName = name;
}
nodes = [TextNode(node.isSilent ? displayName : '@$displayName')];
}
|
@chrisbobbe I have addressed the feedback. PTAL! |
|
Small change: |
chrisbobbe
left a comment
chrisbobbe
left a comment
There was a problem hiding this comment.
Thanks! One nit below, and I'll mark for Greg's review.
lib/api/model/permission.dart
Outdated
| SystemGroupName.everyone => | ||
| zulipLocalizations.systemGroupNameEveryoneIncludingGuests, | ||
| SystemGroupName.members => | ||
| zulipLocalizations.systemGroupNameEveryoneExceptGuests, |
There was a problem hiding this comment.
Let's name the UI strings more closely with the API values they correspond to:
zulipLocalizations.systemGroupNameEveryonezulipLocalizations.systemGroupNameMembers
chrisbobbe
added
integration review
|
Thanks! Looks great except the one nit Chris mentioned above. |
Ruhdee
force-pushed
the
issue-1260
branch
3 times, most recently
from
38c0ffd to
de6b43b
Compare
|
I have renamed the strings and rebased onto main (the latest commit on main caused merge conflicts). PTAL! |
|
Update: Update 2: |
|
Thanks for the revisions! Looks good; merging. Those additions to the commit message are helpful, good thought. |
Fixes zulip#1260. System group names in @-mentions are now rendered using localized strings. These strings match the strings used in the web app code: https://github.com/zulip/zulip/blob/81e0305f9/web/src/settings_config.ts#L1242 Previously, the raw api values returned by the server were shown in system group @-mentions.
3 participants
Fixes #1260.
Changes made:
displayNametoSystemGroupNameto get translated string for current system group.Mentionwidget to fetch system group display name using the current user group mention name (treating the name as api value). If the system group doesn't exist, it falls back to the user group mention name.