Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from Jul 16, 2018

Conversation

@safwanrahman
Copy link
Member

@safwanrahman safwanrahman commented Jul 8, 2018

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?

@safwanrahman safwanrahman self-assigned this Jul 8, 2018
@safwanrahman safwanrahman added this to Backlog in Search update via automation Jul 8, 2018
@safwanrahman safwanrahman moved this from Backlog to In progress in Search update Jul 8, 2018
Copy link
Member

@ericholscher ericholscher left a comment

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.

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>');
Copy link
Member

@ericholscher ericholscher Jul 9, 2018

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.

Copy link
Member Author

@safwanrahman safwanrahman Jul 9, 2018

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?

list_item.append(content);

// Show highlighted texts
if (highlight.content) {
Copy link
Member

@ericholscher ericholscher Jul 9, 2018

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.

Copy link
Member Author

@safwanrahman safwanrahman Jul 9, 2018

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.

Copy link
Contributor

@agjohnson agjohnson left a comment

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.

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

@agjohnson agjohnson Jul 9, 2018

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

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>');
Copy link
Contributor

@agjohnson agjohnson Jul 9, 2018

for consistency, we don't prefix vars with $

var link = doc.link + DOCUMENTATION_OPTIONS.FILE_SUFFIX +
'?highlight=' + $.urlencode(query);

var $item = $('<a>', {'href': link});
Copy link
Contributor

@agjohnson agjohnson Jul 9, 2018

same $ prefix

$list_item.append($item);

// If the document is from subproject, add extra information
if (doc.project !== project) {
Copy link
Contributor

@agjohnson agjohnson Jul 9, 2018

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'

Copy link
Member Author

@safwanrahman safwanrahman Jul 9, 2018

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

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

@agjohnson agjohnson Jul 9, 2018

more $ prefix

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

@agjohnson agjohnson Jul 9, 2018

More $ prefix

@@ -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);
Copy link
Contributor

@agjohnson agjohnson Jul 9, 2018

Debug logging can be removed

Copy link
Contributor

@davidfischer davidfischer left a comment

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
Copy link
Member

@ericholscher ericholscher commented Jul 10, 2018

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

@safwanrahman
Copy link
Member Author

@safwanrahman safwanrahman commented Jul 10, 2018

@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
Copy link
Member Author

@safwanrahman safwanrahman commented Jul 10, 2018

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

@safwanrahman
Copy link
Member Author

@safwanrahman safwanrahman commented Jul 13, 2018

@ericholscher possible to merge?

@ericholscher ericholscher merged commit 512138a into readthedocs:search_upgrade Jul 16, 2018
1 check passed
Search update automation moved this from In progress to Done Jul 16, 2018
safwanrahman added a commit to safwanrahman/readthedocs.org that referenced this issue Jul 16, 2018
[Fix readthedocs#4265] Porting frontend docsearch to work with new API
safwanrahman added a commit to safwanrahman/readthedocs.org that referenced this issue Jul 16, 2018
[Fix readthedocs#4265] Porting frontend docsearch to work with new API
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Search update
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants