[wip] Search pills #9716

shubham-padia · 2018-06-08T18:12:17Z

Immediate TODO:

Fix search icon overlap
Add option of adjusting pill size in input_pill.js.
Fix/(remove if required) the failing casper tests.
Bug where an additional pill is appended when pressing enter, this is due to he text being not cleared before the code handling fr enter key occurs

timabbott · 2018-06-08T18:16:01Z

@showell can you take this review?

showell · 2018-06-09T12:10:48Z

There's a typo in one of the commit messages--search_pil.js.

showell · 2018-06-09T12:16:41Z

static/js/search.js

+        updater: function (search_string) {
+            // Order is important here. narrow_or_search_for_term
+            // assumes that the pill for the selected typeahead has
+            // not been added.


Can you explain why that's the case? If I'm just reading this comment, I don't really know why narrow_or_search_for_term has that restriction, and maybe we can figure out a way so they don't depend on each other? (I think the main thing is just to add to the comment.)

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.
I've also added above text to the comment.

showell · 2018-06-09T12:18:22Z

I did a first pass on this, and the overall code seems really clean. I made a couple small comments, and the build seems to have failed.

showell · 2018-06-09T17:04:07Z

@shubham-padia What is the status of your TODO list in the top comment here? Is there anything we should hold off on merging?

shubham-padia · 2018-06-09T19:06:44Z

@showell I was out for today, I'll get to the todo list tommorow. The first four commits should be good to merge. The commits after that start making changes to the UI, it would be best to merge them once the UI bugs are fixed.

shubham-padia · 2018-06-11T18:55:06Z

@showell I've pushed the following changes:
1.) 3 of the 4 casper tests were failing as $.val() cannot be used for contenteditable divs, $.html() is used in select_item_via_typeahead instead. https://github.com/zulip/zulip/pull/9716/commits/dedb182294eb6ca8a59c6bad0b2752dc7edc6756 incorporates that change. This was was hard to figure out in particular. I was under the impression there was something wrong with the implementations. After trying multiple approaches, the thing that worked for was breaking the function being evaluated in select_item_via_typeahead and taking screenshots in between. I the noticed that no input was being inserted by $.val()
2.) Placing the logic for populating search pills in hashchange.js did not work as expected. narrow.activate calls by top left corner weren't removed once added and one from the stream list were not populated at all. The pre population logic was moved 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.
3.) When pressing enter, two pills were added, one for the item you selected and one search for x where x is the input in the input box. That was fixed in

The failure mentioned in point no.2 exists from dedb182 to b92bc4a. It is fixed in f84b78a. It was not fixable in a single commit and I did not change the history in that case.

TODOs:
1.) Fix search overlap.
2.) Remove tab filters
.

shubham-padia · 2018-06-11T19:02:08Z

@showell I'm curious about the merging policy here. I'm presuming that we will be merging this once the UI bugs are fixes and the UI seems stable. Is that correct?

showell · 2018-06-11T20:24:19Z

@shubham-padia That's correct. If you can set up your branch so that the first N commits are no-brainer merges (i.e. simple refactorings that pass all tests), I would consider opening a separate PR for them called something like "Add prep commits for search pills" and clearly link back to this PR (which already has "wip" in the title--thanks for doing that).

shubham-padia · 2018-06-12T06:27:14Z

Cool :) Although the first 4 commits are mergeable, I'll let them be as it is a file addition and I see no advantage in merging it beforehand.

shubham-padia · 2018-06-16T12:31:11Z

Update: UI changes have been made and are under review on https://chat.zulip.org/#narrow/stream/101-design/topic/search.20pills .
A behavioural TODO is:

taking account of the existing pills for the next suggestion. The current search takes care of that. (e.g if you have a stream filter applied, it will suggest that stream's topic)

shubham-padia · 2018-06-18T22:35:12Z

Changes since the last update:

db3c7e0 Use search strings as the display value for search pills.
35c5091 Fix search icon shifting left along with the first pill on overflow.
808accd Fix both cross icon overlap and its shifting left bug.

@showell @timabbott I've made the changes from the feedback on czo and some other irregularities I encountered. It'd be great to have a review on this. I feel this is a fairly stable experience for now and any other UI/behavourial enhancements should be opened as a follow up.

Final design:

shubham-padia · 2018-06-18T23:09:38Z

Whenever the cross icon was not visible i.e empty search bar out
of focus, the left border of the search bar would appear as if it
was shortened. 8e4c4b2 fixes that.
Before:

After:

shubham-padia · 2018-06-20T17:20:44Z

pinging @showell for review. (I think the previous request for review may have been buried in the multiple emails, please ignore otherwise 😃 )

showell · 2018-06-29T13:29:17Z

In FF I see pills like this (too low in search bar):

showell · 2018-06-29T13:31:52Z

I guess completion now works one pill at a time? It's actually kind of nice for some situations.

This seems to prevent negated search, or at least make it hard to edit.

showell · 2018-06-29T13:34:35Z

Here's another strange situation you can get into:

showell · 2018-06-29T13:36:19Z

If I type "is:private", that turns into a pill, and so far, so good. Then I type "V" and I'm presented with "stream Verona" as an option, which seems wrong, since "is:private" and "stream:Verona" are mutually exclusive options.

I was able to confirm this bug was introduced by your PR, and I believe it has to do with not properly setting base_operators. You can add console statements like below and do before/after:

diff --git a/static/js/search_suggestion.js b/static/js/search_suggestion.js
index 3a20342..d298106 100644
--- a/static/js/search_suggestion.js
+++ b/static/js/search_suggestion.js
@@ -596,6 +596,8 @@ exports.get_suggestions = function (query) {
     suggestions = get_sent_by_me_suggestions(last, base_operators);
     attach_suggestions(result, base, suggestions);

+    console.info('last', last);
+    console.info('base_operators', base_operators);
     suggestions = get_stream_suggestions(last, base_operators);
     attach_suggestions(result, base, suggestions);

shubham-padia · 2018-06-29T13:52:31Z

If I type "is:private", that turns into a pill, and so far, so good. Then I type "V" and I'm presented with "stream Verona" as an option, which seems wrong, since "is:private" and "stream:Verona" are mutually exclusive options.

Yes, the suggestions do not take previous pills into account right now. I was hoping to do that once the current code gets reviewed.
I had mentioned in the behavioural todo above and was going to mention it in the review request, but it got out of my mind 😅.

I'll investigate the other UI bugs.

This setting isn't intended to exist long term, but instead to make it possible to merge our search pills code before we're ready to cut over production environments to use it.

This is preparation for being able to work on the search pills feature without making any user-facing changes until we're ready to enable it.

This way, we can edit the CSS for the searchbox without having to worry about changing the legacy behavior.

Input pills require a contenteditable div with a class named input to fall inside the pill container. On converting the input tag into a div, the size of the input decreases which is compensated by a line-height of 40px. Comment above letter-spacing:normal was removed as chrome and firefox do not change the letter-spacing to normal for a div via the default browser stylesheet. NOTE: Currently writing something into the div will call the action corresponding to that key in the keyboard shortcuts. The input will work fine once the pills have been initiated. For the casper tests, for now, we just use the legacy search code. When we change that, $.val() cannot be used on contenteditable div, so $.html() will need to be used instead in select_item_via_typeahead.

Adds the initialize function to search_pill_widget.js for initializing a pill object on search query box.

The letter-spacing was changed last in commit fc4d80d which is about a 5 year old commit at the point of writing. The change is removed as I did not notice any visual change on removing it. Changing the letter spacing to normal lets the text in the pills be seen legibly, otherwise the characters were overlapping.

This forks off search_legacy.js and search_suggestion_legacy.js so that we can continue running automated tests against the legacy search code while we develop the input pills feature.

Adds an optional parameter `quiet` to removeLastPill and removeAllPills. If `quiet` is a truthy value, the event handler associated with the pill will not be evaluated. This is useful when using clear to reset the 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

tab_bar.js becomes redundant after implementation of search pills. This commit adds a comment to tab_bar.initiliaze, so the event listeners related to it do not get initiated. This does not remove any code related to tab_bar.js. Also adds left and right border around the search icon.

This large function will need to be modified significantly as part of the pills effort, and copying it lets us preserve behavior in production until we're ready to cut things over.

This lets us modify the implementation of this function for the search pills implementation only.

After adding search pills, suggestions were based only on the current input and no validation against the existing pills was done. operator_subset_suggestions have been removed. Default suggestions for base_operators have also been removed. Handle multiple operators: if `is:starred stream:Ver` was typed without selecting the typeahead or pressing enter in between i.e search pill for is:starred has not yet been added, then the description of `is:starred` will act as a prefix in every suggestion. Also makes changes re-enabling person suggestions for names with spaces.

Previously `All messages` was displayed irrespective of the existing pills. Now the suggestion is displayed only if no pills are present

Adds box-shadow to `#searchbox` when either `#search_query` or any of the pills have focus. Uses jquery instead of pure css as the `:focus` event occurs on `#search_query`, while we want to add box-shadow to `#searchbox`. This could have been done with `:focus-within` CSS selector, but it is not supported in IE or Opera. `#search_query` already had an onfocus/focusout listener, adding listeners to `#searchbox.pills` for those events wouldn't have worked as you don't want the focusout event to fire when the focus shifts from input to pill. Also adds `focusin`, `focusout` and `css()` to zjquery. `css` is same as `val`, except it returns an empty object in case of no value instead of an empty string. I don't think `css()` is valid syntax in actual jquery.

`compose_pm_pill.my_pill`, `search_pill_widget.my_pill` and any of its occurrences throughout the app have been replaced to use `widget` instead.

…abled. If search pills are not enabled, the text present in the search bar will be selected on pressing '/' and writing someting without deselecting the text will clear the search text. Since selecting the pills would not make sense in this context, the search box is focused instead.

Pressing `Esc` did not blur a contenteditable div by default, while an input field was blurred by default. Due to this when a user tried to unnarrow using `Esc` key when the searchbox had focus, the focus remained stuck in the div itself and no further action was taken.

shubham-padia · 2018-07-23T18:25:06Z

Update:
@timabbott I've implemented the following items from the second TODO list:

(styling) Remove line to the right of magnifying glass and to the left of cross icon
(styling) Visually show that we're focused on the search area (e.g. a colored border)
Rename my_pill to a clearer name for the group of all search pills
/ hotkey works now, instead of selecting the whole text as it does now, the contenteditable div is focused as selection with pills present didn't make much sense.
Fix Escape hotkey to unnarrow doesn't work if search is focused

timabbott · 2018-07-23T18:40:16Z

I made one change to ef2f6b7, which is to add a return true for the contenteditable case, and merged this. Thanks @shubham-padia!

I also updated the checklist of follow-up items, above, based on your comment. Can you move the remaining to a new issue, that we use as the "remaining search pills implementation details" TODO list? This PR has a long enough history that it's not a great place to keep that.

One other detail worth mentioning: Changes @synicalsyntax made to the user pills ended up breaking some of the styling for this; so you two will want to collaborate to get the search pills styling correct again.

I'm looking forward to this! Aside from the few annoying quirks, this feels a lot nicer :)

shubham-padia · 2018-07-23T18:50:24Z

Opened #10026 for follow-up issues

zulipbot added the size: XL label Jun 8, 2018

showell reviewed Jun 9, 2018

View reviewed changes

shubham-padia force-pushed the spill_clean branch from 34be3b7 to 01f72a6 Compare June 11, 2018 18:31

shubham-padia force-pushed the spill_clean branch 2 times, most recently from 2b2b91b to 3897bd9 Compare June 15, 2018 20:37

shubham-padia force-pushed the spill_clean branch from 8e4c4b2 to c4fa744 Compare June 18, 2018 23:35

shubham-padia mentioned this pull request Jun 21, 2018

search: Add search_pill.js with basic operations. #9794

Merged

shubham-padia force-pushed the spill_clean branch 5 times, most recently from 357615b to a83cca1 Compare June 25, 2018 14:34

timabbott and others added 10 commits July 22, 2018 23:47

search: Add setting to control whether search pills are enabled.

3c1c05f

This setting isn't intended to exist long term, but instead to make it possible to merge our search pills code before we're ready to cut over production environments to use it.

search: Duplicate HTML template content for search area.

9c5cad8

This is preparation for being able to work on the search pills feature without making any user-facing changes until we're ready to enable it.

css: Move message edit history CSS from middle of search CSS.

51c1b6f

css: Put searchbox CSS into a single CSS block.

1695898

search: Copy CSS to a separate #searchbox_legacy block.

7a3e3b8

This way, we can edit the CSS for the searchbox without having to worry about changing the legacy behavior.

search: Initialize search pill widget in ui_init.js.

37d8120

Adds the initialize function to search_pill_widget.js for initializing a pill object on search query box.

search: Duplicate search node tests for pills.

5e40929

This forks off search_legacy.js and search_suggestion_legacy.js so that we can continue running automated tests against the legacy search code while we develop the input pills feature.

shubham-padia force-pushed the spill_clean branch from 4bcc3d2 to 3095435 Compare July 22, 2018 18:18

shubham-padia and others added 6 commits July 23, 2018 03:46

search: Duplicate search_suggestions.get_suggestions.

c90fead

This large function will need to be modified significantly as part of the pills effort, and copying it lets us preserve behavior in production until we're ready to cut things over.

search: Duplicate get_default_suggestion.

8d3816f

This lets us modify the implementation of this function for the search pills implementation only.

search: Do not display All messages suggestion if bar not empty.

14b4165

Previously `All messages` was displayed irrespective of the existing pills. Now the suggestion is displayed only if no pills are present

shubham-padia force-pushed the spill_clean branch 2 times, most recently from 8568d9b to af64797 Compare July 22, 2018 23:57

shubham-padia added 2 commits July 23, 2018 20:39

pills: Use widget instead of my_pill throughout the app.

ca64903

`compose_pm_pill.my_pill`, `search_pill_widget.my_pill` and any of its occurrences throughout the app have been replaced to use `widget` instead.

shubham-padia force-pushed the spill_clean branch from 8ecfac9 to 29d361c Compare July 23, 2018 15:18

shubham-padia added 2 commits July 23, 2018 23:27

shubham-padia force-pushed the spill_clean branch from 29d361c to 13abbc6 Compare July 23, 2018 17:58

timabbott closed this Jul 23, 2018

shubham-padia mentioned this pull request Jul 23, 2018

Follow up issues for search pills #10026

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wip] Search pills #9716

[wip] Search pills #9716

shubham-padia commented Jun 8, 2018 •

edited

timabbott commented Jun 8, 2018

showell commented Jun 9, 2018

showell Jun 9, 2018

shubham-padia Jun 18, 2018

showell commented Jun 9, 2018

showell commented Jun 9, 2018

shubham-padia commented Jun 9, 2018

shubham-padia commented Jun 11, 2018 •

edited

shubham-padia commented Jun 11, 2018

showell commented Jun 11, 2018

shubham-padia commented Jun 12, 2018 •

edited

shubham-padia commented Jun 16, 2018

shubham-padia commented Jun 18, 2018 •

edited

shubham-padia commented Jun 18, 2018

shubham-padia commented Jun 20, 2018

showell commented Jun 29, 2018

showell commented Jun 29, 2018

showell commented Jun 29, 2018

showell commented Jun 29, 2018 •

edited

shubham-padia commented Jun 29, 2018 •

edited

shubham-padia commented Jul 23, 2018

timabbott commented Jul 23, 2018

shubham-padia commented Jul 23, 2018 •

edited

[wip] Search pills #9716

[wip] Search pills #9716

Conversation

shubham-padia commented Jun 8, 2018 • edited

timabbott commented Jun 8, 2018

showell commented Jun 9, 2018

showell Jun 9, 2018

Choose a reason for hiding this comment

shubham-padia Jun 18, 2018

Choose a reason for hiding this comment

showell commented Jun 9, 2018

showell commented Jun 9, 2018

shubham-padia commented Jun 9, 2018

shubham-padia commented Jun 11, 2018 • edited

shubham-padia commented Jun 11, 2018

showell commented Jun 11, 2018

shubham-padia commented Jun 12, 2018 • edited

shubham-padia commented Jun 16, 2018

shubham-padia commented Jun 18, 2018 • edited

shubham-padia commented Jun 18, 2018

shubham-padia commented Jun 20, 2018

showell commented Jun 29, 2018

showell commented Jun 29, 2018

showell commented Jun 29, 2018

showell commented Jun 29, 2018 • edited

shubham-padia commented Jun 29, 2018 • edited

shubham-padia commented Jul 23, 2018

timabbott commented Jul 23, 2018

shubham-padia commented Jul 23, 2018 • edited

shubham-padia commented Jun 8, 2018 •

edited

shubham-padia commented Jun 11, 2018 •

edited

shubham-padia commented Jun 12, 2018 •

edited

shubham-padia commented Jun 18, 2018 •

edited

showell commented Jun 29, 2018 •

edited

shubham-padia commented Jun 29, 2018 •

edited

shubham-padia commented Jul 23, 2018 •

edited