[Fix #4265] Porting frontend docsearch to work with new API #4340

safwanrahman · 2018-07-08T21:46:48Z

This fixes #4265
I have ported the existing search javascript in order to work with the new Restful search API. The API has been implemented in #4309.

@ericholscher @davidfischer r?

ericholscher

Looks good, except for some JS style changes. I don't know JS well enough to check for anything obviously wrong there, but it looks like a pretty simple update to the JS code.

ericholscher · 2018-07-09T09:27:41Z

readthedocs/core/static-src/core/js/doc-embed/search.js

+                    for (var i = 0; i < hit_list.length; i += 1) {
+                        var document = hit_list[i];
+                        var highlight = document.highlight;
+                        var $list_item = $('<li style="display: none;"></li>');


I see what you're doing here, but we don't follow this convention anywhere else in the code, so we shouldn't do it here to be consistent.

I understand that we do not follow the convention. But it actually makes me bit much of confused while writing JS code. without knowing if its a jquery object, its hard to keep track. I am ok to have it like before, but what do you think if we should start following the convention?

ericholscher · 2018-07-09T09:31:28Z

readthedocs/core/static-src/core/js/doc-embed/search.js

-                            list_item.append(content);
+
+                        // Show highlighted texts
+                        if (highlight.content) {


Do we not need to check length here? I don't know JS well enough, but that's what we were doing before, so probably worth keeping it explicit.

In the previous API, the content was provided as a blank list if there are no match in content. but our current API do not provide anything in the content key if there are not anything in the content. So checking the length may raise error.

agjohnson

Looks good so far. I think we can eventually update this interface, but +1 on keeping the logic the same for now.

I've noted some cleanup issues to the javascript, there are some deviations from our current style patterns.

agjohnson · 2018-07-09T16:42:32Z

readthedocs/core/static-src/core/js/doc-embed/search.js

-                                $('<span>')
-                                .text(" (from project " + fields.project + ")")
-                            );
+                    for (var i = 0; i < hit_list.length; i += 1) {


for (var n in hit_list) is a more readable iteration format and is support down to ie6

agjohnson · 2018-07-09T16:43:02Z

readthedocs/core/static-src/core/js/doc-embed/search.js

+                    for (var i = 0; i < hit_list.length; i += 1) {
+                        var doc = hit_list[i];
+                        var highlight = doc.highlight;
+                        var $list_item = $('<li style="display: none;"></li>');


for consistency, we don't prefix vars with $

agjohnson · 2018-07-09T16:43:16Z

readthedocs/core/static-src/core/js/doc-embed/search.js

+                        var link = doc.link + DOCUMENTATION_OPTIONS.FILE_SUFFIX +
+                                   '?highlight=' + $.urlencode(query);
+
+                        var $item = $('<a>', {'href': link});


same $ prefix

agjohnson · 2018-07-09T16:47:04Z

readthedocs/core/static-src/core/js/doc-embed/search.js

+                        $list_item.append($item);
+
+                        // If the document is from subproject, add extra information
+                        if (doc.project !== project) {


Be careful that this doesn't revert to buggy behavior, if the return attributedoc.proejct hasn't changed. The equivalent of doc.project in the current js is always an array, so doc.project !== project is false because doc.project = ['docs'] and project = 'docs'

As discussed over slack, the new API always return String, instead of list.

agjohnson · 2018-07-09T16:47:18Z

readthedocs/core/static-src/core/js/doc-embed/search.js

+                        // If the document is from subproject, add extra information
+                        if (doc.project !== project) {
+                            var text = " (from project " + doc.project + ")";
+                            var $extra = $('<span>', {'text': text});


more $ prefix

agjohnson · 2018-07-09T16:48:03Z

readthedocs/core/static-src/core/js/doc-embed/search.js

+                        // Show highlighted texts
+                        if (highlight.content) {
+                            var content_text = xss(highlight.content[0]);
+                            var $contents = $('<div class="context">');


More $ prefix

agjohnson · 2018-07-09T16:48:32Z

readthedocs/core/static-src/core/js/doc-embed/search.js

@@ -96,11 +96,11 @@ function attach_elastic_search_query(data) {
                withCredentials: true,
            },
            complete: function (resp, status_code) {
-                if (typeof (resp.responseJSON) === 'undefined' ||
-                        typeof (resp.responseJSON.results) === 'undefined') {
+                console.log(status_code);


Debug logging can be removed

davidfischer

There's a lot of conflicts to testing this locally (notably a conflicting migration on projects/0026). Would it be possible to sync up to master?

ericholscher · 2018-07-10T11:36:12Z

👍 on resyncing the search_upgrade branch with master, we can do that after this PR though I think.

safwanrahman · 2018-07-10T11:39:12Z

@davidfischer Yes. there are lot of conflicts as the branch is old. Can you cherry pick your asset generation commits to the branch and generate assets to test?

@ericholscher I am hoping to rebase the search_upgrade branch with master. so the conflict will be gone away! There are big conflict actually that need to be handeled well.

safwanrahman · 2018-07-10T16:40:47Z

@agjohnson I have fixed the issue as you have mentioned. r?

safwanrahman · 2018-07-13T15:16:45Z

@ericholscher possible to merge?

[Fix readthedocs#4265] Porting frontend docsearch to work with new API

Porting frontend docsearch to work with new API

7c682a6

safwanrahman self-assigned this Jul 8, 2018

safwanrahman requested review from ericholscher and davidfischer July 8, 2018 21:47

fixup lint

30f7e74

ericholscher reviewed Jul 9, 2018

View reviewed changes

renaming document to doc`

1fa4db6

agjohnson requested changes Jul 9, 2018

View reviewed changes

davidfischer reviewed Jul 9, 2018

View reviewed changes

remove $ sign

46d72f9

agjohnson approved these changes Jul 12, 2018

View reviewed changes

ericholscher approved these changes Jul 16, 2018

View reviewed changes

ericholscher merged commit 512138a into readthedocs:search_upgrade Jul 16, 2018

safwanrahman pushed a commit to safwanrahman/readthedocs.org that referenced this pull request Jul 16, 2018

Merge pull request readthedocs#4340 from safwanrahman/docsearch

4ed492e

[Fix readthedocs#4265] Porting frontend docsearch to work with new API

safwanrahman pushed a commit to safwanrahman/readthedocs.org that referenced this pull request Jul 16, 2018

Merge pull request readthedocs#4340 from safwanrahman/docsearch

39ada00

[Fix readthedocs#4265] Porting frontend docsearch to work with new API

safwanrahman mentioned this pull request Jul 27, 2018

Port Document search API for Elasticsearch 6.x #4265

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fix #4265] Porting frontend docsearch to work with new API #4340

[Fix #4265] Porting frontend docsearch to work with new API #4340

safwanrahman commented Jul 8, 2018 •

edited

Loading

ericholscher left a comment

ericholscher Jul 9, 2018

safwanrahman Jul 9, 2018

ericholscher Jul 9, 2018

safwanrahman Jul 9, 2018

agjohnson left a comment

agjohnson Jul 9, 2018

agjohnson Jul 9, 2018

agjohnson Jul 9, 2018

agjohnson Jul 9, 2018

safwanrahman Jul 9, 2018

agjohnson Jul 9, 2018

agjohnson Jul 9, 2018

agjohnson Jul 9, 2018

davidfischer left a comment

ericholscher commented Jul 10, 2018

safwanrahman commented Jul 10, 2018

safwanrahman commented Jul 10, 2018

safwanrahman commented Jul 13, 2018

[Fix #4265] Porting frontend docsearch to work with new API #4340

[Fix #4265] Porting frontend docsearch to work with new API #4340

Conversation

safwanrahman commented Jul 8, 2018 • edited Loading

ericholscher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agjohnson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfischer left a comment

Choose a reason for hiding this comment

ericholscher commented Jul 10, 2018

safwanrahman commented Jul 10, 2018

safwanrahman commented Jul 10, 2018

safwanrahman commented Jul 13, 2018

safwanrahman commented Jul 8, 2018 •

edited

Loading