From 36707a33cabad1525e4c657a83dbe5166cf3af48 Mon Sep 17 00:00:00 2001 From: Shubham Padia Date: Sat, 14 Jul 2018 19:40:00 +0530 Subject: [PATCH] search: Add a basic implementation of search pills. Following points have been implemented in this commit: 1.) Add search pill on selecting typeahead. 2.) Re-narrow after removing a search pill. 3.) Add quiet optional parameter to removeLastPill. 4.) Pre populate search pills in narrow.activate. 5.) Clear existing search pills on narrow.deactivate. Description of above points: 1.) I tried out using the description from suggestions.lookup_table to append a pill using appendValidatedData so that the description had not to be calculated again. But the description in the suggestions lookup contains html due to highlighting. This html is escaped when inputed in a pill. An attempt was also made to remove the higlighting by replacing the tags. But other espaced characters like < also popped up, so it was better to use append_search_string. 3.) If one wants to refresh the pill using pill.clear and wants to repopulate them, evaluating the event_handler associated with the action of removing the pill may not be desired. 4.) Pill population code is added to narrow.activate. Pills are not populated if the narrow was triggered by search as search handles the addition and removal of pill by itself. The reason for not handling search too in narrow.activate is to avoid clearing the pills and repopulating them. Example of some of the triggers for narrow.activate include `restore draft`, `topic change`,`sidebar`. Also modifies tests for search.js --- frontend_tests/node_tests/narrow_activate.js | 7 ++ frontend_tests/node_tests/search.js | 47 ++++++++----- frontend_tests/node_tests/search_pill.js | 14 ++-- static/js/narrow.js | 13 ++++ static/js/search.js | 35 +++++++++- static/js/search_pill.js | 8 +-- static/js/search_pill_widget.js | 6 ++ static/styles/zulip.scss | 71 ++++++++++++-------- templates/zerver/app/navbar.html | 6 +- 9 files changed, 145 insertions(+), 62 deletions(-) diff --git a/frontend_tests/node_tests/narrow_activate.js b/frontend_tests/node_tests/narrow_activate.js index 403ca05c9f..79fd209c19 100644 --- a/frontend_tests/node_tests/narrow_activate.js +++ b/frontend_tests/node_tests/narrow_activate.js @@ -6,6 +6,7 @@ zrequire('Filter', 'js/filter'); zrequire('MessageListData', 'js/message_list_data'); zrequire('unread'); zrequire('narrow'); +zrequire('search_pill'); set_global('blueslip', {}); set_global('channel', {}); @@ -24,6 +25,12 @@ set_global('stream_list', {}); set_global('top_left_corner', {}); set_global('ui_util', {}); set_global('unread_ops', {}); +set_global('search_pill_widget', { + my_pill: { + clear: function () {return true;}, + appendValue: function () {return true;}, + }, +}); var noop = () => {}; diff --git a/frontend_tests/node_tests/search.js b/frontend_tests/node_tests/search.js index b082d71d1e..fd7ed3d07a 100644 --- a/frontend_tests/node_tests/search.js +++ b/frontend_tests/node_tests/search.js @@ -2,6 +2,10 @@ set_global('page_params', { search_pills_enabled: true, }); zrequire('search'); +zrequire('search_pill'); +zrequire('util'); +zrequire('Filter', 'js/filter'); +zrequire('search_pill_widget'); const noop = () => {}; const return_true = () => true; @@ -14,7 +18,8 @@ set_global('ui_util', { change_tab_to: noop, }); set_global('narrow', {}); -set_global('Filter', {}); + +search_pill.append_search_string = noop; global.patch_builtin('setTimeout', func => func()); @@ -105,21 +110,25 @@ run_test('initizalize', () => { { let operators; let is_blurred; + let is_append_search_string_called; search_query_box.blur = () => { is_blurred = true; }; + search_pill.append_search_string = () => { + is_append_search_string_called = true; + }; /* Test updater */ const _setup = (search_box_val) => { is_blurred = false; + is_append_search_string_called = false; search_query_box.val(search_box_val); - Filter.parse = (search_string) => { - assert.equal(search_string, search_box_val); - return operators; - }; narrow.activate = (raw_operators, options) => { assert.deepEqual(raw_operators, operators); assert.deepEqual(options, {trigger: 'search'}); }; + search_pill.get_search_string_for_current_filter = () => { + return ''; + }; }; operators = [{ @@ -128,8 +137,9 @@ run_test('initizalize', () => { operand: 'ver', }]; _setup('ver'); - assert.equal(opts.updater('ver'), 'ver'); + opts.updater('ver'); assert(is_blurred); + assert(is_append_search_string_called); operators = [{ negated: false, @@ -137,13 +147,15 @@ run_test('initizalize', () => { operand: 'Verona', }]; _setup('stream:Verona'); - assert.equal(opts.updater('stream:Verona'), 'stream:Verona'); + opts.updater('stream:Verona'); assert(is_blurred); + assert(is_append_search_string_called); search.is_using_input_method = true; _setup('stream:Verona'); - assert.equal(opts.updater('stream:Verona'), 'stream:Verona'); + opts.updater('stream:Verona'); assert(!is_blurred); + assert(is_append_search_string_called); } }; @@ -177,22 +189,17 @@ run_test('initizalize', () => { is_blurred = false; search_button.prop('disabled', false); search_query_box.val(search_box_val); - Filter.parse = (search_string) => { - assert.equal(search_string, search_box_val); - return operators; - }; narrow.activate = (raw_operators, options) => { assert.deepEqual(raw_operators, operators); assert.deepEqual(options, {trigger: 'search'}); }; + search_pill.get_search_string_for_current_filter = () => { + return ''; + }; }; - operators = [{ - negated: false, - operator: 'search', - operand: '', - }]; + operators = []; _setup(''); ev.which = 15; @@ -215,7 +222,11 @@ run_test('initizalize', () => { assert(is_blurred); assert(search_button.prop('disabled')); - + operators = [{ + negated: false, + operator: 'search', + operand: 'ver', + }]; _setup('ver'); search.is_using_input_method = true; func(ev); diff --git a/frontend_tests/node_tests/search_pill.js b/frontend_tests/node_tests/search_pill.js index ff916094ca..3fe4f097c5 100644 --- a/frontend_tests/node_tests/search_pill.js +++ b/frontend_tests/node_tests/search_pill.js @@ -5,13 +5,13 @@ zrequire('Filter', 'js/filter'); zrequire('Handlebars', 'handlebars'); var is_starred_item = { - display_value: 'starred messages', - search_string: 'is:starred', + display_value: 'is:starred', + description: 'starred messages', }; var is_private_item = { - display_value: 'private messages', - search_string: 'is:private', + display_value: 'is:private', + description: 'private messages', }; run_test('create_item', () => { @@ -34,7 +34,7 @@ run_test('append', () => { function fake_append(search_string) { appended = true; - assert.equal(search_string, is_starred_item.search_string); + assert.equal(search_string, is_starred_item.display_value); } function fake_clear() { @@ -46,7 +46,7 @@ run_test('append', () => { clear_text: fake_clear, }; - search_pill.append_search_string(is_starred_item.search_string, pill_widget); + search_pill.append_search_string(is_starred_item.display_value, pill_widget); assert(appended); assert(cleared); @@ -60,7 +60,7 @@ run_test('get_items', () => { }; assert.deepEqual(search_pill.get_search_string_for_current_filter(pill_widget), - is_starred_item.search_string + ' ' + is_private_item.search_string); + is_starred_item.display_value + ' ' + is_private_item.display_value); }); run_test('create_pills', () => { diff --git a/static/js/narrow.js b/static/js/narrow.js index a43eb7d9de..b15bb0bd7f 100644 --- a/static/js/narrow.js +++ b/static/js/narrow.js @@ -245,6 +245,14 @@ exports.activate = function (raw_operators, opts) { hashchange.save_narrow(operators); } + if (page_params.search_pills_enabled && opts.trigger !== 'search') { + search_pill_widget.my_pill.clear(true); + _.each(operators, function (operator) { + var search_string = Filter.unparse([operator]); + search_pill.append_search_string(search_string, search_pill_widget.my_pill); + }); + } + // Put the narrow operators in the search bar. $('#search_query').val(Filter.unparse(operators)); search.update_button_visibility(); @@ -655,6 +663,11 @@ exports.deactivate = function () { compose_fade.update_message_list(); + // clear existing search pills + if (page_params.search_pills_enabled) { + search_pill_widget.my_pill.clear(true); + } + top_left_corner.handle_narrow_deactivated(); stream_list.handle_narrow_deactivated(); diff --git a/static/js/search.js b/static/js/search.js index ef7265a42f..1262ccc686 100644 --- a/static/js/search.js +++ b/static/js/search.js @@ -14,7 +14,20 @@ function narrow_or_search_for_term(search_string) { return search_query_box.val(); } ui_util.change_tab_to('#home'); - var operators = Filter.parse(search_string); + + var operators; + if (page_params.search_pills_enabled) { + // search_string only contains the suggestion selected + // from the typeahead. base_query stores the query + // corresponding to the existing pills. + var base_query = search_pill.get_search_string_for_current_filter( + search_pill_widget.my_pill); + var base_operators = Filter.parse(base_query); + var suggestion_operator = Filter.parse(search_string); + operators = base_operators.concat(suggestion_operator); + } else { + operators = Filter.parse(search_string); + } narrow.activate(operators, {trigger: 'search'}); // It's sort of annoying that this is not in a position to @@ -74,10 +87,28 @@ exports.initialize = function () { matcher: function () { return true; }, - updater: narrow_or_search_for_term, + updater: function (search_string) { + // Order is important here. narrow_or_search_for_term + // gets a search string from existing pills and obtains + // existing operators. Newly selected suggestion is added + // to those operators. If narrow_or_search_for_term was + // called after append_search_string, the existing search + // pills at the time for calling that function would also + // have the newly selected suggestion, and appending it again + // would cause duplication. + var result = narrow_or_search_for_term(search_string); + if (page_params.search_pills_enabled) { + search_pill.append_search_string(search_string, + search_pill_widget.my_pill); + $("#search_query").focus(); + } else { + return result; + } + }, sorter: function (items) { return items; }, + stopAdvance: page_params.search_pills_enabled, }); searchbox_form.on('compositionend', function () { diff --git a/static/js/search_pill.js b/static/js/search_pill.js index 2b13cb96f9..4bcc14b761 100644 --- a/static/js/search_pill.js +++ b/static/js/search_pill.js @@ -5,13 +5,13 @@ exports.create_item_from_search_string = function (search_string) { var operator = Filter.parse(search_string); var description = Filter.describe(operator); return { - display_value: description, - search_string: search_string, + display_value: search_string, + description: description, }; }; exports.get_search_string_from_item = function (item) { - return item.search_string; + return item.display_value; }; exports.create_pills = function (pill_container) { @@ -32,7 +32,7 @@ exports.append_search_string = function (search_string, pill_widget) { exports.get_search_string_for_current_filter = function (pill_widget) { var items = pill_widget.items(); - var search_strings = _.pluck(items, 'search_string'); + var search_strings = _.pluck(items, 'display_value'); return search_strings.join(' '); }; diff --git a/static/js/search_pill_widget.js b/static/js/search_pill_widget.js index 711fa443aa..6c51cf0c6a 100644 --- a/static/js/search_pill_widget.js +++ b/static/js/search_pill_widget.js @@ -8,6 +8,12 @@ exports.initialize = function () { } var container = $('#search_arrows'); exports.my_pill = search_pill.create_pills(container); + + exports.my_pill.onPillRemove(function () { + var base_query = search_pill.get_search_string_for_current_filter(exports.my_pill); + var operators = Filter.parse(base_query); + narrow.activate(operators, {trigger: 'search'}); + }); }; return exports; diff --git a/static/styles/zulip.scss b/static/styles/zulip.scss index e1e70d7c94..16ca493a4c 100644 --- a/static/styles/zulip.scss +++ b/static/styles/zulip.scss @@ -1855,30 +1855,24 @@ nav a .no-style { } #searchbox { + display: flex; width: 100%; height: 40px; + border-right: 1px solid #e0e0e0; .navbar-search { margin-top: 0px; - width: auto; + width: calc(100% - 66px); float: none; overflow: hidden; - border-right: 1px solid hsl(0, 0%, 87.8%); height: 40px; } .input-append { + align-items: center; + display: flex; position: relative; - width: 100%; - - .fa-search { - padding: 0px; - font-size: 20px; - position: absolute; - left: 10px; - top: 10px; - z-index: 5; - } + width: calc(100% - 28px); } #search_query { @@ -1886,7 +1880,7 @@ nav a .no-style { font-size: 16px; height: 40px; padding: 0px; - padding-left: 35px; + padding-left: 5px; padding-right: 20px; border: none; border-radius: 0px; @@ -1895,20 +1889,11 @@ nav a .no-style { line-height: 40px; } - #search_query:focus { - box-shadow: inset 0px 0px 0px 2px hsl(204, 20%, 74%); - } - .search_button, .search_button[disabled]:hover { - position: absolute; - right: 2px; - top: 6px; + position: relative; background: none; - border-radius: 0px; - border: none; - height: 30px; - text-align: center; + height: 40px; padding: 4px; color: hsl(0, 0%, 80%); font-size: 18px; @@ -1917,6 +1902,7 @@ nav a .no-style { -moz-box-shadow: none; text-shadow: none; z-index: 5; + float: right; } .search_button:hover { @@ -1928,11 +1914,19 @@ nav a .no-style { } a.search_icon { + display: table; + height: 100%; color: hsl(0, 0%, 80%); text-decoration: none; - display: block; - width: 1px; - height: 1px; + padding: 0 10px; + font-size: 20px; + z-index: 5; + float: left; + + i { + display: table-cell; + vertical-align: middle; + } } a.search_icon:hover { @@ -1942,7 +1936,28 @@ nav a .no-style { #search_arrows { font-size: 90%; - letter-spacing: normal + letter-spacing: normal; + border: none; + } + + @media (min-width: 500px) { + .pill { + padding: 2px 18px 2px 3px !important; + font-size: 14px; + + .exit { + top: 2px !important; /* pill's top padding + border */ + } + } + } + @media (max-width: 500px) { + #search_arrows .pill { + line-height: 20px; + + .exit { + top: 0; + } + } } } diff --git a/templates/zerver/app/navbar.html b/templates/zerver/app/navbar.html index 69a7986cd3..56598ac046 100644 --- a/templates/zerver/app/navbar.html +++ b/templates/zerver/app/navbar.html @@ -48,15 +48,15 @@ {% else %}