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

Tag suggestions #4034

Merged
merged 1 commit into from
Nov 9, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 4 additions & 3 deletions gulpfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,10 @@ var vendorBundles = {
jquery: ['jquery'],
bootstrap: ['bootstrap'],
raven: ['raven-js'],
unorm: ['unorm'],
};
var vendorModules = ['jquery', 'bootstrap', 'raven-js'];
var vendorNoParseModules = ['jquery'];
var vendorModules = ['jquery', 'bootstrap', 'raven-js', 'unorm'];
var vendorNoParseModules = ['jquery', 'unorm'];

// Builds the bundles containing vendor JS code
gulp.task('build-vendor-js', function () {
Expand Down Expand Up @@ -252,7 +253,7 @@ function runKarma(baseConfig, opts, done) {
client: {
mocha: {
grep: taskArgs.grep,
}
},
},
};

Expand Down
3 changes: 3 additions & 0 deletions h/assets.ini
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ site_js =
header_js =
scripts/header.bundle.js

search_js =
scripts/unorm.bundle.js

site_css =
styles/site.css
styles/icomoon.css
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,10 @@ class AutosuggestDropdownController extends Controller {
});

this._setList(configOptions.list);


// Public API
this.setHeader = this._setHeader;
Copy link
Member

Choose a reason for hiding this comment

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

Any reason not to just rename _setHeader to setHeader?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd prefer to not mix internals and public api. Or rather, make it super obvious which is which

}

update(newState, prevState){
Expand Down Expand Up @@ -206,7 +210,7 @@ class AutosuggestDropdownController extends Controller {

if (selection){
this.options.onSelect(selection);
this._toggleSuggestionsVisibility(/*show*/false);
this._filterAndToggleVisibility();
this.setState({
activeId: null,
});
Expand Down
275 changes: 179 additions & 96 deletions h/static/scripts/controllers/search-bar-controller.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,16 @@
'use strict';

var escapeHtml = require('escape-html');
Copy link
Member

Choose a reason for hiding this comment

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

Need a new line after the vendor bundle list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol I did that and then removed it thinking you would say not to have two lines :P


var Controller = require('../base/controller');
var LozengeController = require('./lozenge-controller');
var AutosuggestDropdownController = require('./autosuggest-dropdown-controller');
var SearchTextParser = require('../util/search-text-parser');
var stringUtil = require('../util/string');

const FACET_TYPE = 'FACET';
const TAG_TYPE = 'TAG';
const MAX_SUGGESTIONS = 5;

/**
* Controller for the search bar.
Expand All @@ -15,107 +22,11 @@ class SearchBarController extends Controller {
this._input = this.refs.searchBarInput;
this._lozengeContainer = this.refs.searchBarLozenges;

let explanationList = [
{
title: 'user:',
explanation: 'search by username',
},
{
title: 'tag:',
explanation: 'search for annotations with a tag',
},
{
title: 'url:',
explanation: 'see all annotations on a page',
},
{
title: 'group:',
explanation: 'show annotations created in a group you are a member of',
},
];

var selectFacet = facet => {
this._input.value = facet;

setTimeout(()=>{
this._input.focus();
}, 0);
};


var getTrimmedInputValue = () => {
return this._input.value.trim();
};

new AutosuggestDropdownController( this._input, {

list: explanationList,

header: 'Narrow your search',

classNames: {
container: 'search-bar__dropdown-menu-container',
header: 'search-bar__dropdown-menu-header',
list: 'search-bar__dropdown-menu',
item: 'search-bar__dropdown-menu-item',
activeItem: 'js-search-bar-dropdown-menu-item--active',
},

renderListItem: (listItem)=>{

let itemContents = `<span class="search-bar__dropdown-menu-title"> ${listItem.title} </span>`;

if (listItem.explanation){
itemContents += `<span class="search-bar__dropdown-menu-explanation"> ${listItem.explanation} </span>`;
}

return itemContents;
},

listFilter: function(list, currentInput){

currentInput = (currentInput || '').trim();

return list.filter((item)=>{

if (!currentInput){
return item;
} else if (currentInput.indexOf(':') > -1) {
return false;
}
return item.title.toLowerCase().indexOf(currentInput) >= 0;

}).sort((a,b)=>{

// this sort functions intention is to
// sort partial matches as lower index match
// value first. Then let natural sort of the
// original list take effect if they have equal
// index values or there is no current input value

if (!currentInput){
return 0;
}

let aIndex = a.title.indexOf(currentInput);
let bIndex = b.title.indexOf(currentInput);

if (aIndex > bIndex){
return 1;
} else if (aIndex < bIndex){
return -1;
}
return 0;
});
},

onSelect: (itemSelected)=>{
selectFacet(itemSelected.title);
},

});


/**
* Insert a hidden <input> with an empty value into the search <form>.
*
Expand Down Expand Up @@ -207,12 +118,184 @@ class SearchBarController extends Controller {
}
};


this._hiddenInput = insertHiddenInput(this.refs.searchBarForm);

let explanationList = [
{
matchOn: 'user',
title: 'user:',
explanation: 'search by username',
},
{
matchOn: 'tag',
title: 'tag:',
explanation: 'search for annotations with a tag',
},
{
matchOn: 'url',
title: 'url:',
explanation: 'see all annotations on a page',
},
{
matchOn: 'group',
title: 'group:',
explanation: 'show annotations created in a group you are a member of',
},
].map((item)=>{ return Object.assign(item, { type: FACET_TYPE}); });

// tagSuggestions are made available by the scoped template data.
// see search.html.jinja2 for definition
const tagSuggestionJSON = document.querySelector('.js-tag-suggestions');
let tagSuggestions = [];

if(tagSuggestionJSON){
try{
tagSuggestions = JSON.parse(tagSuggestionJSON.innerHTML.trim());
}catch(e){
console.error('Could not parse .js-tag-suggestions JSON content', e);
Copy link
Member

Choose a reason for hiding this comment

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

If finding the tag succeeds but parsing the JSON fails this will lead to some confusing errors below because .map will be called on an Element. I would suggest pulling this out into a helper function which takes the selector for the suggestions tag and returns Object|Array|null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just changed the logic to not reuse a variable. I don't like doing that for this reason- didn't think about it while jumping around though.

}
}

let tagsList = ((tagSuggestions) || []).map((item)=>{
return Object.assign(item, {
type: TAG_TYPE,
title: escapeHtml(item.tag), // make safe
Copy link
Member

Choose a reason for hiding this comment

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

This is fine, although I'm concerned that there will be a mixup with escaping in future, where something escaped for HTML is incorrectly used in a different context or some transformation gets applied to the string which breaks the escaping (on the Python side, this hazard is avoided by wrapping escaped strings using a special Markup class).

matchOn: stringUtil.fold(stringUtil.normalize(item.tag)),
usageCount: item.count || 0,
});
});


this._suggestionsHandler = new AutosuggestDropdownController( this._input, {

list: explanationList.concat(tagsList),

header: 'Narrow your search',

classNames: {
container: 'search-bar__dropdown-menu-container',
header: 'search-bar__dropdown-menu-header',
list: 'search-bar__dropdown-menu',
item: 'search-bar__dropdown-menu-item',
activeItem: 'js-search-bar-dropdown-menu-item--active',
},

renderListItem: (listItem)=>{

let itemContents = `<span class="search-bar__dropdown-menu-title"> ${listItem.title} </span>`;

if (listItem.explanation){
itemContents += `<span class="search-bar__dropdown-menu-explanation"> ${listItem.explanation} </span>`;
}

return itemContents;
},

listFilter: (list, currentInput)=>{

currentInput = (currentInput || '').trim();

let typeFilter = currentInput.indexOf('tag:') === 0 ? TAG_TYPE : FACET_TYPE;
let inputFilter = stringUtil.fold(stringUtil.normalize(currentInput));

if(typeFilter === TAG_TYPE){
inputFilter = inputFilter.substr(/*'tag:' len*/4);

// remove the initial quote for comparisons if it exists
if(inputFilter[0] === '\'' || inputFilter[0] === '"'){
inputFilter = inputFilter.substr(1);
}
}

if(this.state.suggestionsType !== typeFilter){
this.setState({
suggestionsType: typeFilter,
});
}

return list.filter((item)=>{
return item.type === typeFilter && item.matchOn.toLowerCase().indexOf(inputFilter.toLowerCase()) >= 0;
}).sort((a,b)=>{

// this sort functions intention is to
// sort partial matches as lower index match
// value first. Then let natural sort of the
// original list take effect if they have equal
// index values or there is no current input value

if (inputFilter){
let aIndex = a.matchOn.indexOf(inputFilter);
let bIndex = b.matchOn.indexOf(inputFilter);

// match score
if (aIndex > bIndex){
return 1;
} else if (aIndex < bIndex){
return -1;
}
}


// If we are filtering on tags, we need to arrange
// by popularity
if(typeFilter === TAG_TYPE){
if(a.usageCount > b.usageCount){
return -1;
}else if(a.usageCount < b.usageCount) {
return 1;
}
}

return 0;

}).slice(0, MAX_SUGGESTIONS);
},

onSelect: (itemSelected)=>{

if (itemSelected.type === TAG_TYPE){
let tagSelection = itemSelected.title;

// wrap multi word phrases with quotes to keep
// autosuggestions consistent with what user needs to do
if(tagSelection.indexOf(' ') > -1){
tagSelection = `"${tagSelection}"`;
}

addLozenge('tag:' + tagSelection);
this._input.value = '';
} else {
this._input.value = itemSelected.title;
setTimeout(()=>{
this._input.focus();
}, 0);
}
updateHiddenInput();
},

});

this._input.addEventListener('keydown', onInputKeyDown);
this._input.addEventListener('input', updateHiddenInput);
lozengifyInput();
}

update(newState, prevState){

if(!this._suggestionsHandler){
return;
}

if(newState.suggestionsType !== prevState.suggestionsType){
if(newState.suggestionsType === TAG_TYPE){
this._suggestionsHandler.setHeader('Popular tags');
}else {
this._suggestionsHandler.setHeader('Narrow your search');
}
}

}
}

module.exports = SearchBarController;
Original file line number Diff line number Diff line change
Expand Up @@ -341,8 +341,6 @@ describe('AutosuggestDropdownController', function () {
assert.propertyVal(selectedItem, 'title', 'tag:');
assert.propertyVal(selectedItem, 'explanation', 'search for annotations with a tag');

assert.isFalse(isSuggestionContainerVisible(), 'post select hide');

assert.isFalse(form.onsubmit.called, 'should not submit the form on enter');

done();
Expand Down
Loading