From 00e74d0de257343fc49d252ecff6b58676bdae5a Mon Sep 17 00:00:00 2001 From: evykassirer Date: Thu, 1 Aug 2024 20:58:10 -0700 Subject: [PATCH] search: Move group user remove logic out of input_pill. --- web/src/input_pill.ts | 78 ++++++++---------------------------- web/src/search_pill.ts | 51 ++++++++++++++++++++++- web/tests/input_pill.test.js | 3 -- 3 files changed, 66 insertions(+), 66 deletions(-) diff --git a/web/src/input_pill.ts b/web/src/input_pill.ts index d1ee28299a..ff7e793057 100644 --- a/web/src/input_pill.ts +++ b/web/src/input_pill.ts @@ -6,7 +6,6 @@ import assert from "minimalistic-assert"; import render_input_pill from "../templates/input_pill.hbs"; import * as keydown_util from "./keydown_util"; -import type {SearchUserPill} from "./search_pill"; import * as ui_util from "./ui_util"; // See https://zulip.readthedocs.io/en/latest/subsystems/input-pills.html @@ -28,9 +27,14 @@ type InputPillCreateOptions = { get_text_from_item: (item: ItemType) => string; get_display_value_from_item: (item: ItemType) => string; generate_pill_html?: (item: ItemType) => string; + on_pill_exit?: ( + clicked_pill: HTMLElement, + all_pills: InputPill[], + remove_pill: (pill: HTMLElement) => void, + ) => void; }; -type InputPill = { +export type InputPill = { item: ItemType; $element: JQuery; }; @@ -45,6 +49,7 @@ type InputPillStore = { get_text_from_item: InputPillCreateOptions["get_text_from_item"]; get_display_value_from_item: InputPillCreateOptions["get_display_value_from_item"]; generate_pill_html: InputPillCreateOptions["generate_pill_html"]; + on_pill_exit: InputPillCreateOptions["on_pill_exit"]; onPillCreate?: () => void; onPillRemove?: (pill: InputPill, trigger: RemovePillTrigger) => void; createPillonPaste?: () => void; @@ -89,6 +94,7 @@ export function create( 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, + on_pill_exit: opts.on_pill_exit, }; // a dictionary of internal functions. Some of these are exposed as well, @@ -205,52 +211,6 @@ export function create( return undefined; }, - // TODO: This function is only used for the search input supporting multiple user - // pills within an individual top-level pill. Ideally, we'd encapsulate it in a - // subclass used only for search so that this code can be part of search_pill.ts. - removeUserPill(user_container: HTMLElement, user_id: number, trigger: RemovePillTrigger) { - // First get the outer pill that contains the user pills. - let container_idx: number | undefined; - for (let x = 0; x < store.pills.length; x += 1) { - if (store.pills[x]!.$element[0] === user_container) { - container_idx = x; - } - } - assert(container_idx !== undefined); - assert(store.pills[container_idx]!.item.type === "search_user"); - // TODO: Figure out how to get this typed correctly. - // eslint-disable-next-line @typescript-eslint/consistent-type-assertions - const user_pill_container = store.pills[container_idx]! - .item as unknown as SearchUserPill; - - // If there's only one user in this pill, delete the whole pill. - if (user_pill_container.users.length === 1) { - assert(user_pill_container.users[0]!.user_id === user_id); - this.removePill(user_container, trigger); - return; - } - - // Remove the user id from the pill data. - let user_idx: number | undefined; - for (let x = 0; x < user_pill_container.users.length; x += 1) { - if (user_pill_container.users[x]!.user_id === user_id) { - user_idx = x; - } - } - assert(user_idx !== undefined); - user_pill_container.users.splice(user_idx, 1); - - // Remove the user pill from the DOM. - const $user_pill = $(store.pills[container_idx]!.$element.children(".pill")[user_idx]!); - assert($user_pill.data("user-id") === user_id); - $user_pill.remove(); - - // This is needed to run the "change" event handler registered in - // compose_recipient.js, which calls the `update_on_recipient_change` to update - // the compose_fade state. - store.$input.trigger("change"); - }, - // this will remove the last pill in the container -- by default tied // to the "Backspace" key when the value of the input is empty. // If quiet is a truthy value, the event handler associated with the @@ -446,20 +406,14 @@ export function create( // when the "×" is clicked on a pill, it should delete that pill and then // select the input field. store.$parent.on("click", ".exit", function (this: HTMLElement, e) { - const $user_pill_container = $(this).parents(".user-pill-container"); - if ($user_pill_container.length) { - // The user-pill-container container class is used exclusively for - // group-DM search pills, where multiple user pills sit inside a larger - // pill. The exit icons in those individual user pills should remove - // just that pill, not the outer pill. - // TODO: Figure out how to move this code into search_pill.ts. - const user_id = $(this).closest(".pill").attr("data-user-id"); - assert(user_id !== undefined); - funcs.removeUserPill( - $user_pill_container[0]!, - Number.parseInt(user_id, 10), - "close", - ); + if (store.on_pill_exit) { + store.on_pill_exit(this, store.pills, (pill: HTMLElement): void => { + funcs.removePill(pill, "close"); + }); + // This is needed to run the "change" event handler registered in + // compose_recipient.js, which calls the `update_on_recipient_change` to update + // the compose_fade state. + store.$input.trigger("change"); } else { e.stopPropagation(); const $pill = $(this).closest(".pill"); diff --git a/web/src/search_pill.ts b/web/src/search_pill.ts index 533e162ae2..9963cbb7b4 100644 --- a/web/src/search_pill.ts +++ b/web/src/search_pill.ts @@ -6,7 +6,7 @@ import render_search_user_pill from "../templates/search_user_pill.hbs"; import {Filter} from "./filter"; import * as input_pill from "./input_pill"; -import type {InputPillContainer} from "./input_pill"; +import type {InputPill, InputPillContainer} from "./input_pill"; import * as people from "./people"; import type {User} from "./people"; import type {NarrowTerm} from "./state_data"; @@ -60,6 +60,54 @@ export function get_search_string_from_item(item: SearchPill): string { return `${sign}${item.operator}: ${get_search_operand(item)}`; } +// This is called when the a pill is closed. We have custom logic here +// because group user pills have pills inside of them, and it's possible +// to e.g. remove a user from a group-DM pill without deleting the whole +// DM pill. +function on_pill_exit( + clicked_pill: HTMLElement, + all_pills: InputPill[], + remove_pill: (pill: HTMLElement) => void, +): void { + const $user_pill_container = $(clicked_pill).parents(".user-pill-container"); + if (!$user_pill_container.length) { + // This is just a regular search pill, so we don't need to do fancy logic. + remove_pill(clicked_pill); + return; + } + // The user-pill-container container class is used exclusively for + // group-DM search pills, where multiple user pills sit inside a larger + // pill. The exit icons in those individual user pills should remove + // just that pill, not the outer pill. + const user_id_string = $(clicked_pill).closest(".pill").attr("data-user-id"); + assert(user_id_string !== undefined); + const user_id = Number.parseInt(user_id_string, 10); + + // First get the outer pill that contains the user pills. + const outer_idx = all_pills.findIndex((pill) => pill.$element[0] === $user_pill_container[0]); + assert(outer_idx !== -1); + const user_container_pill = all_pills[outer_idx]!.item; + assert(user_container_pill?.type === "search_user"); + + // If there's only one user in this pill, delete the whole pill. + if (user_container_pill.users.length === 1) { + assert(user_container_pill.users[0]!.user_id === user_id); + remove_pill($user_pill_container[0]!); + return; + } + + // Remove the user id from the pill data. + const user_idx = user_container_pill.users.findIndex((user) => user.user_id === user_id); + assert(user_idx !== -1); + user_container_pill.users.splice(user_idx, 1); + + // Remove the user pill from the DOM. + const $outer_container = all_pills[outer_idx]!.$element; + const $user_pill = $($outer_container.children(".pill")[user_idx]!); + assert($user_pill.attr("data-user-id") === user_id.toString()); + $user_pill.remove(); +} + export function create_pills($pill_container: JQuery): SearchPillWidget { const pills = input_pill.create({ $container: $pill_container, @@ -77,6 +125,7 @@ export function create_pills($pill_container: JQuery): SearchPillWidget { }); }, get_display_value_from_item: get_search_string_from_item, + on_pill_exit, }); // We don't automatically create pills on paste. When the user // presses enter, we validate the input then. diff --git a/web/tests/input_pill.test.js b/web/tests/input_pill.test.js index a6bfa8c0e0..a7428fa4af 100644 --- a/web/tests/input_pill.test.js +++ b/web/tests/input_pill.test.js @@ -485,9 +485,6 @@ run_test("exit button on pill", ({mock_template}) => { assert.equal(sel, ".pill"); return $curr_pill_stub; }, - parents() { - return []; - }, }), };