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.

Sign up for GitHub

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
+35 −36
Merged

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

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7c682a6
Porting frontend docsearch to work with new API
safwanrahman Jul 8, 2018
30f7e74
fixup lint
safwanrahman Jul 8, 2018
1fa4db6
renaming document to doc`
safwanrahman Jul 9, 2018
46d72f9
remove $ sign
safwanrahman Jul 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 39 additions & 39 deletions readthedocs/core/static-src/core/js/doc-embed/search.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,48 +23,48 @@ function attach_elastic_search_query(data) {

search_url.href = api_host;
search_url.pathname = '/api/v2/docsearch/';
search_url.search = '?q=' + $.urlencode(query) + '&project=' + project +
'&version=' + version + '&language=' + language;
search_url.search = '?query=' + $.urlencode(query) + '&project=' + project +
'&version=' + version + '&language=' + language;

search_def
.then(function (results) {
var hits = results.hits || {};
var hit_list = hits.hits || [];
.then(function (data) {
var hit_list = data.results || [];
var total_count = data.count || 0;

if (hit_list.length) {
for (var n in hit_list) {
var hit = hit_list[n];
var fields = hit.fields || {};
var list_item = $('<li style="display: none;"></li>');
var item_url = document.createElement('a');
var highlight = hit.highlight;

item_url.href += fields.link +
DOCUMENTATION_OPTIONS.FILE_SUFFIX;
item_url.search = '?highlight=' + $.urlencode(query);

// Result list elements
list_item.append(
$('<a />')
.attr('href', item_url)
.html(fields.title)
);
// fields.project is returned as an array
if (fields.project.indexOf(project) === -1) {
list_item.append(
$('<span>')
.text(" (from project " + fields.project + ")")
);
for (var i = 0; i < hit_list.length; i += 1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

var doc = hit_list[i];
var highlight = doc.highlight;
var $list_item = $('<li style="display: none;"></li>');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for consistency, we don't prefix vars with $


// Creating the result from elements
var link = doc.link + DOCUMENTATION_OPTIONS.FILE_SUFFIX +
'?highlight=' + $.urlencode(query);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same $ prefix

$item.html(doc.title);
$list_item.append($item);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

var text = " (from project " + doc.project + ")";
var $extra = $('<span>', {'text': text});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more $ prefix


$list_item.append($extra);
}
if (highlight.content.length) {
var content = $('<div class="context">')
.html(xss(highlight.content[0]));
content.find('em').addClass('highlighted');
list_item.append(content);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More $ prefix


$contents.html(content_text);
$contents.find('em').addClass('highlighted');
$list_item.append($contents);
}

Search.output.append(list_item);
list_item.slideDown(5);
Search.output.append($list_item);
$list_item.slideDown(5);
}
}

Expand All @@ -74,7 +74,7 @@ function attach_elastic_search_query(data) {
}
else {
Search.status.text(
_('Search finished, found %s page(s) matching the search query.').replace('%s', hit_list.length)
_('Search finished, found %s page(s) matching the search query.').replace('%s', total_count)
);
}
})
Expand All @@ -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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug logging can be removed

if (status_code !== 'success' || resp.responseJSON.count === 0) {
return search_def.reject();
}
return search_def.resolve(resp.responseJSON.results);
return search_def.resolve(resp.responseJSON);
}
})
.error(function (resp, status_code, error) {
Expand Down