Mirror
/
zulip
mirror of https://github.com/zulip/zulip.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

input_pill: Remove display_value in favor of module-specific values. 

This commit is part of a bigger project to remove custom logic in
the input_pill module. This commit move us away from a world where
we have a `display_value` that's used as identifying information
for a pill as well as for what we display to the user. Now individual
widgets can configure how they come up with a display value based
on the data of that type of pill.

Note: The change in the stream pill test for setting subscribers
for denmark is fixing an issue that wasn't discoverable before.
There always should have been three subscribers.
This commit is contained in:
evykassirer 2024-07-29 21:13:31 -07:00 committed by Tim Abbott
parent 7dfb928277
commit 6e47d851ec
18 changed files with 111 additions and 97 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
docs/subsystems/input-pills.md
View File

@ -23,6 +23,7 @@ var pills = input_pill.create({
$container: $pill_container,
create_item_from_text: user_pill.create_item_from_email,
get_text_from_item: user_pill.get_email_from_item,
get_display_value_from_item: user_pill.get_display_value_from_item,
});
```
@ -30,8 +31,7 @@ You can look at `web/src/user_pill.ts` to see how the above
methods are implemented. Essentially you just need to convert
from raw data (like an email) to structured data (like an object
with display_value, email, and user_id for a user), and vice
versa. The most important field to supply is `display_value`.
For user pills `pill_item.display_value === user.full_name`.
versa.
## Typeahead

15
web/src/add_subscribers_pill.ts
View File

@ -66,10 +66,20 @@ function set_up_pill_typeahead({
pill_typeahead.set_up_combined($pill_container.find(".input"), pill_widget, opts);
}
function get_display_value_from_item(item: InputPillItem<CombinedPill>): string {
if (item.type === "user_group") {
return user_group_pill.display_pill(user_groups.get_user_group_from_id(item.group_id));
} else if (item.type === "stream") {
return stream_pill.get_display_value_from_item(item);
}
assert(item.type === "user");
return user_pill.get_display_value_from_item(item);
}
function generate_pill_html(item: InputPillItem<CombinedPill>): string {
if (item.type === "user_group") {
return render_input_pill({
display_value: item.display_value,
display_value: get_display_value_from_item(item),
group_id: item.group_id,
});
} else if (item.type === "user") {
@ -79,6 +89,7 @@ function generate_pill_html(item: InputPillItem<CombinedPill>): string {
return render_input_pill({
...item,
has_stream: true,
display_value: get_display_value_from_item(item),
});
}
@ -93,6 +104,7 @@ export function create({
$container: $pill_container,
create_item_from_text,
get_text_from_item,
get_display_value_from_item,
generate_pill_html,
});
function get_users(): User[] {
@ -138,6 +150,7 @@ export function create_without_add_button({
$container: $pill_container,
create_item_from_text,
get_text_from_item,
get_display_value_from_item,
generate_pill_html,
});
function get_users(): User[] {

1
web/src/compose_pm_pill.ts
View File

@ -22,6 +22,7 @@ export function initialize_pill(): UserPillWidget {
pill_config,
create_item_from_text: user_pill.create_item_from_email,
get_text_from_item: user_pill.get_email_from_item,
get_display_value_from_item: user_pill.get_display_value_from_item,
generate_pill_html: (item: InputPillItem<UserPill>) =>
user_pill.generate_pill_html(item, true),
});

2
web/src/email_pill.ts
View File

@ -24,7 +24,6 @@ export function create_item_from_email(
return {
type: "email",
display_value: email,
email,
};
}
@ -52,6 +51,7 @@ export function create_pills(
pill_config,
create_item_from_text: create_item_from_email,
get_text_from_item: get_email_from_item,
get_display_value_from_item: get_email_from_item,
});
return pill_container;
}

31
web/src/input_pill.ts
View File

@ -13,10 +13,7 @@ import * as ui_util from "./ui_util";
// See https://zulip.readthedocs.io/en/latest/subsystems/input-pills.html
export type InputPillItem<ItemType> = {
display_value: string;
type: string;
// Used for search pills
operator?: string;
} & ItemType;
export type InputPillConfig = {
@ -34,6 +31,7 @@ type InputPillCreateOptions<ItemType> = {
pill_config?: InputPillConfig | undefined,
) => InputPillItem<ItemType> | undefined;
get_text_from_item: (item: InputPillItem<ItemType>) => string;
get_display_value_from_item: (item: InputPillItem<ItemType>) => string;
generate_pill_html?: (item: InputPillItem<ItemType>) => string;
};
@ -50,6 +48,7 @@ type InputPillStore<ItemType> = {
$input: JQuery;
create_item_from_text: InputPillCreateOptions<ItemType>["create_item_from_text"];
get_text_from_item: InputPillCreateOptions<ItemType>["get_text_from_item"];
get_display_value_from_item: InputPillCreateOptions<ItemType>["get_display_value_from_item"];
generate_pill_html: InputPillCreateOptions<ItemType>["generate_pill_html"];
onPillCreate?: () => void;
onPillRemove?: (pill: InputPill<ItemType>, trigger: RemovePillTrigger) => void;
@ -58,10 +57,6 @@ type InputPillStore<ItemType> = {
convert_to_pill_on_enter: boolean;
};
type InputPillRenderingDetails = {
display_value: string;
};
// These are the functions that are exposed to other modules.
export type InputPillContainer<ItemType> = {
appendValue: (text: string) => void;
@ -95,6 +90,7 @@ export function create<ItemType>(
$input: opts.$container.find(".input").expectOne(),
create_item_from_text: opts.create_item_from_text,
get_text_from_item: opts.get_text_from_item,
get_display_value_from_item: opts.get_display_value_from_item,
split_text_on_comma: opts.split_text_on_comma ?? true,
convert_to_pill_on_enter: opts.convert_to_pill_on_enter ?? true,
generate_pill_html: opts.generate_pill_html,
@ -134,7 +130,7 @@ export function create<ItemType>(
const existing_items = funcs.items();
const item = store.create_item_from_text(text, existing_items, store.pill_config);
if (!item?.display_value) {
if (!item) {
store.$input.addClass("shake");
return undefined;
}
@ -145,11 +141,6 @@ export function create<ItemType>(
// This is generally called by typeahead logic, where we have all
// the data we need (as opposed to, say, just a user-typed email).
appendValidatedData(item: InputPillItem<ItemType>) {
if (!item.display_value) {
blueslip.error("no display_value returned");
return;
}
if (!item.type) {
blueslip.error("no type defined for the item");
return;
@ -158,10 +149,9 @@ export function create<ItemType>(
if (store.generate_pill_html !== undefined) {
pill_html = store.generate_pill_html(item);
} else {
const opts: InputPillRenderingDetails = {
display_value: item.display_value,
};
pill_html = render_input_pill(opts);
pill_html = render_input_pill({
display_value: store.get_display_value_from_item(item),
});
}
const payload: InputPill<ItemType> = {
item,
@ -258,13 +248,6 @@ export function create<ItemType>(
}
assert(user_idx !== undefined);
user_pill_container.users.splice(user_idx, 1);
const sign = user_pill_container.negated ? "-" : "";
const search_string =
sign +
user_pill_container.operator +
":" +
user_pill_container.users.map((user) => user.email).join(",");
user_pill_container.display_value = search_string;
// Remove the user pill from the DOM.
const $user_pill = $(store.pills[container_idx]!.$element.children(".pill")[user_idx]!);

1
web/src/invite.ts
View File

@ -346,6 +346,7 @@ function open_invite_user_modal(e: JQuery.ClickEvent<Document, undefined>): void
$container: $pill_container,
create_item_from_text: email_pill.create_item_from_email,
get_text_from_item: email_pill.get_email_from_item,
get_display_value_from_item: email_pill.get_email_from_item,
});
$("#invite-user-modal .dialog_submit_button").prop("disabled", true);

3
web/src/invite_stream_picker_pill.ts
View File

@ -55,8 +55,9 @@ export function create($stream_pill_container: JQuery): stream_pill.StreamPillWi
render_input_pill({
...item,
has_stream: true,
display_value: stream_pill.get_display_string_from_item(item),
display_value: stream_pill.get_display_value_from_item(item),
}),
get_display_value_from_item: stream_pill.get_display_value_from_item,
});
add_default_stream_pills(pill_widget);
set_up_pill_typeahead({pill_widget, $pill_container: $stream_pill_container});

11
web/src/search_pill.ts
View File

@ -16,11 +16,6 @@ import type {UserStatusEmojiInfo} from "./user_status";
export type SearchUserPill = {
type: "search_user";
operator: string;
// TODO: It would be nice if we just call this `search_string` instead of
// `display_value`, because we don't actually display this value for user
// pills, but `display_value` is needed to hook into the generic input pill
// logic and it would be a decent amount of work to change that.
display_value: string;
negated: boolean;
users: {
display_value: string;
@ -36,7 +31,6 @@ export type SearchUserPill = {
type SearchPill =
| {
type: "search";
display_value: string;
operator: string;
operand: string;
negated: boolean | undefined;
@ -54,7 +48,6 @@ export function create_item_from_search_string(search_string: string): SearchPil
return undefined;
}
return {
display_value: search_string,
type: "search",
operator: search_term.operator,
operand: search_term.operand,
@ -83,6 +76,7 @@ export function create_pills($pill_container: JQuery): SearchPillWidget {
display_value,
});
},
get_display_value_from_item: get_search_string_from_item,
});
// We don't automatically create pills on paste. When the user
// presses enter, we validate the input then.
@ -96,12 +90,9 @@ function append_user_pill(
operator: string,
negated: boolean,
): void {
const sign = negated ? "-" : "";
const search_string = sign + operator + ":" + users.map((user) => user.email).join(",");
const pill_data: SearchUserPill = {
type: "search_user",
operator,
display_value: search_string,
negated,
users: users.map((user) => ({
display_value: user.full_name,

25
web/src/stream_pill.ts
View File

@ -1,3 +1,5 @@
import assert from "minimalistic-assert";
import {$t} from "./i18n";
import type {InputPillContainer, InputPillItem} from "./input_pill";
import * as peer_data from "./peer_data";
@ -8,6 +10,7 @@ import type {CombinedPillContainer, CombinedPillItem} from "./typeahead_helper";
export type StreamPill = {
type: "stream";
stream: StreamSubscription;
show_subscriber_count: boolean;
};
export type StreamPillWidget = InputPillContainer<StreamPill>;
@ -55,14 +58,9 @@ export function create_item_from_stream_name(
return undefined;
}
let display_value = sub.name;
if (show_subscriber_count) {
display_value = format_stream_name_and_subscriber_count(sub);
}
return {
type: "stream",
display_value,
show_subscriber_count,
stream: sub,
};
}
@ -82,18 +80,23 @@ export function get_user_ids(pill_widget: StreamPillWidget | CombinedPillContain
return user_ids;
}
export function get_display_value_from_item(item: StreamPill): string {
const stream = stream_data.get_sub_by_id(item.stream.stream_id);
assert(stream !== undefined);
if (item.show_subscriber_count) {
return format_stream_name_and_subscriber_count(stream);
}
return stream.name;
}
export function append_stream(
stream: StreamSubscription,
pill_widget: StreamPillWidget | CombinedPillContainer,
show_subscriber_count = true,
): void {
let display_value = stream.name;
if (show_subscriber_count) {
display_value = format_stream_name_and_subscriber_count(stream);
}
pill_widget.appendValidatedData({
type: "stream",
display_value,
show_subscriber_count,
stream,
});
pill_widget.clear_text();

4
web/src/user_group_pill.ts
View File

@ -18,7 +18,7 @@ export type UserGroupPillData = UserGroup & {
is_silent?: boolean;
};
function display_pill(group: UserGroup): string {
export function display_pill(group: UserGroup): string {
const group_members = get_group_members(group);
return $t_html(
{defaultMessage: "{group_name}: {group_size, plural, one {# user} other {# users}}"},
@ -42,7 +42,6 @@ export function create_item_from_group_name(
return {
type: "user_group",
display_value: display_pill(group),
group_id: group.id,
group_name: group.name,
};
@ -76,7 +75,6 @@ function get_group_members(user_group: UserGroup): number[] {
export function append_user_group(group: UserGroup, pill_widget: CombinedPillContainer): void {
pill_widget.appendValidatedData({
type: "user_group",
display_value: display_pill(group),
group_id: group.id,
group_name: group.name,
});

16
web/src/user_pill.ts
View File

@ -17,6 +17,7 @@ export type UserPill = {
type: "user";
user_id?: number;
email: string;
full_name: string | undefined;
img_src?: string;
deactivated?: boolean;
status_emoji_info?: (EmojiRenderingDetails & {emoji_alt_code?: boolean}) | undefined; // TODO: Move this in user_status.js
@ -46,7 +47,7 @@ export function create_item_from_email(
// is the email itself.
return {
type: "user",
display_value: email,
full_name: undefined,
email,
};
}
@ -67,11 +68,9 @@ export function create_item_from_email(
const status_emoji_info = user_status.get_status_emoji(user.user_id);
// We must supply display_value for the widget to work. Everything
// else is for our own use in callbacks.
const item: InputPillItem<UserPill> = {
type: "user",
display_value: user.full_name,
full_name: user.full_name,
user_id: user.user_id,
email: user.email,
img_src: avatar_url,
@ -108,7 +107,7 @@ export function append_person(opts: {
const pill_data: InputPillItem<UserPill> = {
type: "user",
display_value: person.full_name,
full_name: person.full_name,
user_id: person.user_id,
email: person.email,
img_src: avatar_url,
@ -166,6 +165,10 @@ export function append_user(user: User, pills: UserPillWidget | CombinedPillCont
}
}
export function get_display_value_from_item(item: UserPill): string {
return item.full_name ?? item.email;
}
export function generate_pill_html(
item: InputPillItem<UserPill>,
show_user_status_emoji = false,
@ -179,7 +182,7 @@ export function generate_pill_html(
}
}
return render_input_pill({
display_value: item.display_value,
display_value: get_display_value_from_item(item),
has_image: item.img_src !== undefined,
deactivated: item.deactivated,
should_add_guest_user_indicator: item.should_add_guest_user_indicator,
@ -199,6 +202,7 @@ export function create_pills(
pill_config,
create_item_from_text: create_item_from_email,
get_text_from_item: get_email_from_item,
get_display_value_from_item,
generate_pill_html,
});
return pills;

4
web/tests/compose_pm_pill.test.js
View File

@ -114,7 +114,7 @@ run_test("pills", ({override, override_rewire}) => {
assert.ok(get_by_email_called);
assert.equal(typeof res, "object");
assert.equal(res.user_id, iago.user_id);
assert.equal(res.display_value, iago.full_name);
assert.equal(res.full_name, iago.full_name);
})();
(function test_deactivated_pill() {
@ -124,7 +124,7 @@ run_test("pills", ({override, override_rewire}) => {
assert.ok(get_by_email_called);
assert.equal(typeof res, "object");
assert.equal(res.user_id, iago.user_id);
assert.equal(res.display_value, iago.full_name);
assert.equal(res.full_name, iago.full_name);
assert.ok(res.deactivated);
people.add_active_user(iago);
})();

3
web/tests/composebox_typeahead.test.js
View File

@ -50,6 +50,7 @@ const typeahead_helper = zrequire("typeahead_helper");
const muted_users = zrequire("muted_users");
const people = zrequire("people");
const user_groups = zrequire("user_groups");
const user_pill = zrequire("user_pill");
const stream_data = zrequire("stream_data");
const stream_list_sort = zrequire("stream_list_sort");
const compose_pm_pill = zrequire("compose_pm_pill");
@ -846,7 +847,7 @@ test("initialize", ({override, override_rewire, mock_template}) => {
onPillCreate() {},
onPillRemove() {},
appendValidatedData(item) {
appended_names.push(item.display_value);
appended_names.push(user_pill.get_display_value_from_item(item));
},
}));
compose_pm_pill.initialize({

25
web/tests/input_pill.test.js
View File

@ -42,12 +42,13 @@ run_test("basics", ({mock_template}) => {
$container,
create_item_from_text: noop,
get_text_from_item: noop,
get_display_value_from_item: (item) => item.language,
});
// type for a pill can be any string but it needs to be
// defined while creating any pill.
const item = {
display_value: "JavaScript",
language: "JavaScript",
type: "language",
};
@ -68,19 +69,19 @@ run_test("basics", ({mock_template}) => {
function set_up() {
const items = {
blue: {
display_value: "BLUE",
color_name: "BLUE",
description: "color of the sky",
type: "color",
},
red: {
display_value: "RED",
color_name: "RED",
type: "color",
description: "color of stop signs",
},
yellow: {
display_value: "YELLOW",
color_name: "YELLOW",
type: "color",
description: "color of bananas",
},
@ -99,7 +100,8 @@ function set_up() {
const config = {
$container,
create_item_from_text,
get_text_from_item: (item) => item.display_value,
get_text_from_item: (item) => item.color_name,
get_display_value_from_item: (item) => item.color_name,
};
return {
@ -403,7 +405,7 @@ run_test("insert_remove", ({mock_template}) => {
const pills = widget._get_pills_for_testing();
for (const pill of pills) {
pill.$element.remove = set_colored_removed_func(pill.item.display_value);
pill.$element.remove = set_colored_removed_func(pill.item.color_name);
}
let key_handler = $container.get_on_handler("keydown", ".input");
@ -527,12 +529,8 @@ run_test("misc things", () => {
assert.ok(shake_class_removed);
// bad data
blueslip.expect("error", "no display_value returned");
widget.appendValidatedData("this-has-no-item-attribute");
blueslip.expect("error", "no type defined for the item");
widget.appendValidatedData({
display_value: "This item has no type.",
language: "js",
});
@ -566,8 +564,9 @@ run_test("appendValue/clear", ({mock_template}) => {
const config = {
$container,
create_item_from_text: (s) => ({type: "color", display_value: s}),
get_text_from_item: /* istanbul ignore next */ (s) => s.display_value,
create_item_from_text: (s) => ({type: "color", color_name: s}),
get_text_from_item: /* istanbul ignore next */ (s) => s.color_name,
get_display_value_from_item: (s) => s.color_name,
};
$pill_input.before = noop;
@ -587,7 +586,7 @@ run_test("appendValue/clear", ({mock_template}) => {
const removed_colors = [];
for (const pill of pills) {
pill.$element.remove = () => {
removed_colors.push(pill.item.display_value);
removed_colors.push(pill.item.color_name);
};
}

18
web/tests/pill_typeahead.test.js
View File

@ -137,10 +137,7 @@ run_test("set_up_user", ({mock_template, override, override_rewire}) => {
sort_recipients_called = true;
return users;
});
mock_template("input_pill.hbs", true, (data, html) => {
assert.equal(typeof data.display_value, "string");
return html;
});
mock_template("input_pill.hbs", true, (_data, html) => html);
let input_pill_typeahead_called = false;
const $fake_input = $.create(".input");
$fake_input.before = noop;
@ -152,6 +149,7 @@ run_test("set_up_user", ({mock_template, override, override_rewire}) => {
$container,
create_item_from_text: noop,
get_text_from_item: noop,
get_display_value_from_item: noop,
});
let update_func_called = false;
@ -228,10 +226,7 @@ run_test("set_up_stream", ({mock_template, override, override_rewire}) => {
sort_streams_called = true;
return streams;
});
mock_template("input_pill.hbs", true, (data, html) => {
assert.equal(typeof data.display_value, "string");
return html;
});
mock_template("input_pill.hbs", true, (_data, html) => html);
let input_pill_typeahead_called = false;
const $fake_input = $.create(".input");
$fake_input.before = noop;
@ -243,6 +238,7 @@ run_test("set_up_stream", ({mock_template, override, override_rewire}) => {
$container,
create_item_from_text: noop,
get_text_from_item: noop,
get_display_value_from_item: noop,
});
let update_func_called = false;
@ -315,10 +311,7 @@ run_test("set_up_stream", ({mock_template, override, override_rewire}) => {
run_test("set_up_combined", ({mock_template, override, override_rewire}) => {
override_typeahead_helper(override_rewire);
mock_template("input_pill.hbs", true, (data, html) => {
assert.equal(typeof data.display_value, "string");
return html;
});
mock_template("input_pill.hbs", true, (_data, html) => html);
let input_pill_typeahead_called = false;
const $fake_input = $.create(".input");
$fake_input.before = noop;
@ -330,6 +323,7 @@ run_test("set_up_combined", ({mock_template, override, override_rewire}) => {
$container,
create_item_from_text: noop,
get_text_from_item: noop,
get_display_value_from_item: noop,
});
let update_func_called = false;

19
web/tests/stream_pill.test.js
View File

@ -28,18 +28,18 @@ const germany = {
invite_only: true,
};
peer_data.set_subscribers(denmark.stream_id, [1, 2, 3, 77]);
peer_data.set_subscribers(denmark.stream_id, [1, 2, 77]);
peer_data.set_subscribers(sweden.stream_id, [1, 2, 3, 4, 5]);
const denmark_pill = {
type: "stream",
display_value: "Denmark: 3 users",
stream: denmark,
show_subscriber_count: true,
};
const sweden_pill = {
type: "stream",
display_value: "translated: Sweden: 5 users",
stream: sweden,
show_subscriber_count: true,
};
const subs = [denmark, sweden, germany];
@ -82,6 +82,19 @@ run_test("create_item", () => {
test_create_item("#germany", [], undefined, true, stream_data.get_invite_stream_data);
});
run_test("display_value", () => {
assert.deepEqual(
stream_pill.get_display_value_from_item(denmark_pill),
"translated: Denmark: 3 users",
);
assert.deepEqual(
stream_pill.get_display_value_from_item(sweden_pill),
"translated: Sweden: 5 users",
);
sweden_pill.show_subscriber_count = false;
assert.deepEqual(stream_pill.get_display_value_from_item(sweden_pill), "Sweden");
});
run_test("get_stream_id", () => {
assert.equal(stream_pill.get_stream_name_from_item(denmark_pill), denmark.name);
});

18
web/tests/user_group_pill.test.js
View File

@ -68,13 +68,11 @@ const admins_pill = {
group_id: admins.id,
group_name: admins.name,
type: "user_group",
display_value: "translated HTML: " + admins.name + ": " + admins.members.length + " users",
};
const testers_pill = {
group_id: testers.id,
group_name: testers.name,
type: "user_group",
display_value: "translated HTML: " + testers.name + ": " + testers.members.length + " users",
};
const everyone_pill = {
group_id: everyone.id,
@ -83,7 +81,6 @@ const everyone_pill = {
// While we can programmatically set the user count below,
// calculating it would almost mimic the entire display function
// here, reducing the usefulness of the test.
display_value: "translated HTML: translated: Everyone: 5 users",
};
const groups = [admins, testers, everyone];
@ -104,6 +101,21 @@ run_test("create_item", () => {
test_create_item("role:everyone", [], everyone_pill);
});
run_test("display_value", () => {
assert.deepEqual(
user_group_pill.display_pill(admins),
"translated HTML: " + admins.name + ": " + admins.members.length + " users",
);
assert.deepEqual(
user_group_pill.display_pill(testers),
"translated HTML: " + testers.name + ": " + testers.members.length + " users",
);
assert.deepEqual(
user_group_pill.display_pill(everyone),
"translated HTML: translated: Everyone: 5 users",
);
});
run_test("get_stream_id", () => {
assert.equal(user_group_pill.get_group_name_from_item(admins_pill), admins.name);
});

8
web/tests/user_pill.test.js
View File

@ -27,13 +27,13 @@ const isaac = {
const bogus_item = {
email: "bogus@example.com",
type: "user",
display_value: "bogus@example.com",
full_name: undefined,
// status_emoji_info: undefined,
};
const isaac_item = {
email: "isaac@example.com",
display_value: "Isaac Newton",
full_name: "Isaac Newton",
type: "user",
user_id: isaac.user_id,
deactivated: false,
@ -46,7 +46,7 @@ const inaccessible_user_id = 103;
const inaccessible_user_item = {
email: "user103@example.com",
display_value: "translated: Unknown user",
full_name: "translated: Unknown user",
type: "user",
user_id: inaccessible_user_id,
deactivated: false,
@ -108,7 +108,7 @@ test("append", () => {
function fake_append(opts) {
appended = true;
assert.equal(opts.email, isaac.email);
assert.equal(opts.display_value, isaac.full_name);
assert.equal(opts.full_name, isaac.full_name);
assert.equal(opts.user_id, isaac.user_id);
assert.equal(opts.img_src, isaac_item.img_src);
}