From fc6b4ff6aa90969164296c0c3f39064be7e08cf9 Mon Sep 17 00:00:00 2001 From: Anders Kaseorg Date: Sun, 10 Jul 2022 19:27:00 -0700 Subject: [PATCH] input_pill: Identify pills by DOM nodes, not random IDs. Signed-off-by: Anders Kaseorg --- frontend_tests/node_tests/input_pill.js | 18 ++++++------------ frontend_tests/node_tests/search_future.js | 2 +- static/js/input_pill.js | 22 ++++++++++------------ static/js/search.js | 4 ++-- 4 files changed, 19 insertions(+), 27 deletions(-) diff --git a/frontend_tests/node_tests/input_pill.js b/frontend_tests/node_tests/input_pill.js index 539fc276f5..e7eb8efb3c 100644 --- a/frontend_tests/node_tests/input_pill.js +++ b/frontend_tests/node_tests/input_pill.js @@ -161,6 +161,7 @@ function set_up() { run_test("copy from pill", ({override_rewire, mock_template}) => { mock_template("input_pill.hbs", true, (data, html) => { assert.ok(["BLUE", "RED"].includes(data.display_value)); + $(html)[0] = ``; return html; }); @@ -177,10 +178,7 @@ run_test("copy from pill", ({override_rewire, mock_template}) => { let copied_text; const $pill_stub = { - data: (field) => { - assert.equal(field, "id"); - return "some_id2"; - }, + [0]: "" }; const e = { @@ -408,6 +406,7 @@ run_test("insert_remove", ({override_rewire, mock_template}) => { mock_template("input_pill.hbs", true, (data, html) => { assert.equal(typeof data.display_value, "string"); assert.ok(html.startsWith, "
`; return html; }); @@ -496,10 +495,7 @@ run_test("insert_remove", ({override_rewire, mock_template}) => { const $focus_pill_stub = { next: () => $next_pill_stub, - data: (field) => { - assert.equal(field, "id"); - return "some_id1"; - }, + [0]: "", }; $container.set_find_results(".pill:focus", $focus_pill_stub); @@ -518,6 +514,7 @@ run_test("exit button on pill", ({override_rewire, mock_template}) => { mock_template("input_pill.hbs", true, (data, html) => { assert.equal(typeof data.display_value, "string"); assert.ok(html.startsWith, "
`; return html; }); @@ -549,10 +546,7 @@ run_test("exit button on pill", ({override_rewire, mock_template}) => { const $curr_pill_stub = { next: () => $next_pill_stub, - data: (field) => { - assert.equal(field, "id"); - return "some_id1"; - }, + [0]: "", }; const exit_button_stub = { diff --git a/frontend_tests/node_tests/search_future.js b/frontend_tests/node_tests/search_future.js index 850c85094d..279baa0331 100644 --- a/frontend_tests/node_tests/search_future.js +++ b/frontend_tests/node_tests/search_future.js @@ -17,7 +17,7 @@ const search_suggestion = mock_esm("../../static/js/search_suggestion"); mock_esm("../../static/js/search_pill_widget", { widget: { - getByID: () => true, + getByElement: () => true, }, }); mock_esm("../../static/js/ui_util", { diff --git a/static/js/input_pill.js b/static/js/input_pill.js index ccd480f0d2..f362233cb2 100644 --- a/static/js/input_pill.js +++ b/static/js/input_pill.js @@ -152,14 +152,14 @@ export function create(opts) { return true; }, - // this searches given a particular pill ID for it, removes the node + // this searches given the DOM node for a pill, removes the node // from the DOM, removes it from the array and returns it. // this would generally be used for DOM-provoked actions, such as a user // clicking on a pill to remove it. - removePill(id) { + removePill(element) { let idx; for (let x = 0; x < store.pills.length; x += 1) { - if (store.pills[x].id === id) { + if (store.pills[x].$element[0] === element) { idx = x; } } @@ -228,8 +228,8 @@ export function create(opts) { return drafts.length === 0; }, - getByID(id) { - return store.pills.find((pill) => pill.id === id); + getByElement(element) { + return store.pills.find((pill) => pill.$element[0] === element); }, _get_pills_for_testing() { @@ -324,8 +324,7 @@ export function create(opts) { break; case "Backspace": { const $next = $pill.next(); - const id = $pill.data("id"); - funcs.removePill(id); + funcs.removePill($pill[0]); $next.trigger("focus"); // the "Backspace" key in Firefox will go back a page if you do // not prevent it. @@ -363,9 +362,8 @@ export function create(opts) { e.stopPropagation(); const $pill = $(this).closest(".pill"); const $next = $pill.next(); - const id = $pill.data("id"); - funcs.removePill(id); + funcs.removePill($pill[0]); $next.trigger("focus"); compose.update_fade(); @@ -378,8 +376,8 @@ export function create(opts) { }); store.$parent.on("copy", ".pill", (e) => { - const id = store.$parent.find(":focus").data("id"); - const data = funcs.getByID(id); + const $element = store.$parent.find(":focus"); + const data = funcs.getByElement($element[0]); e.originalEvent.clipboardData.setData( "text/plain", store.get_text_from_item(data.item), @@ -393,7 +391,7 @@ export function create(opts) { appendValue: funcs.appendPill.bind(funcs), appendValidatedData: funcs.appendValidatedData.bind(funcs), - getByID: funcs.getByID, + getByElement: funcs.getByElement, items: funcs.items, onPillCreate(callback) { diff --git a/static/js/search.js b/static/js/search.js index 9476117007..22eb6b71f2 100644 --- a/static/js/search.js +++ b/static/js/search.js @@ -184,8 +184,8 @@ export function initialize() { // really it would be OK if they did). if (page_params.search_pills_enabled) { - const pill_id = $(e.relatedTarget).closest(".pill").data("id"); - const search_pill = search_pill_widget.widget.getByID(pill_id); + const $element = $(e.relatedTarget).closest(".pill"); + const search_pill = search_pill_widget.widget.getByElement($element[0]); if (search_pill) { // The searchbox loses focus while the search // pill element gains focus.