Commit b8ad9722 authored by Lyza Danger Gardner's avatar Lyza Danger Gardner Committed by Lyza Gardner

Suggested tag matching should not use lookback

Change the matching logic for determining whether a given suggested tag
(`tag`) matches the currently-typed tag field text (`query`).

Formerly, `tag` would match `query` if `tag` were a substring of `query`
in any position. However, this creates a lot of noise in the matches and
is not commonly what folks are after.

Now, `tag` will match `query` if:

* `tag` starts with `query` OR
* `query` occurs within `tag` after a word boundary or a non-word
   character

Thus, a `query` of "app":

* Would match the `tag` "apple"
* Would match the `tag` "crab apple" ("app" occurs after a word boundary)
* Would match the `tag` "crab.apple" ("app" occurs after non-word char)
* Would NOT match the `tag` "crabapple"
parent 1cc4e1ca
......@@ -27,8 +27,15 @@ export default function tags(localStorage) {
function filter(query, limit = null) {
const savedTags = localStorage.getObject(TAGS_LIST_KEY) || [];
let resultCount = 0;
return savedTags.filter(e => {
if (e.toLowerCase().indexOf(query.toLowerCase()) !== -1) {
// query will match tag if:
// * tag starts with query (e.g. tag "banana" matches query "ban"), OR
// * any word in the tag starts with query
// (e.g. tag "pink banana" matches query "ban"), OR
// * tag has substring query occurring after a non-word character
// (e.g. tag "pink!banana" matches query "ban")
let regex = new RegExp('(\\W|\\b)' + query, 'i');
return savedTags.filter(tag => {
if (tag.match(regex)) {
if (limit === null || resultCount < limit) {
// limit allows a subset of the results
// See https://github.com/hypothesis/client/issues/1606
......
......@@ -17,7 +17,7 @@ class FakeStorage {
}
}
describe('sidebar.tags', () => {
describe('sidebar/services/tags', () => {
let fakeLocalStorage;
let tags;
......@@ -36,6 +36,16 @@ describe('sidebar.tags', () => {
count: 5,
updated: stamp,
},
'bar argon': {
text: 'bar argon',
count: 2,
updated: stamp,
},
banana: {
text: 'banana',
count: 2,
updated: stamp,
},
future: {
text: 'future',
count: 2,
......@@ -56,18 +66,22 @@ describe('sidebar.tags', () => {
});
describe('#filter', () => {
it('returns tags having the query as a substring', () => {
assert.deepEqual(tags.filter('a'), ['bar', 'argon']);
it('returns tags that start with the query string', () => {
assert.deepEqual(tags.filter('b'), ['bar', 'bar argon', 'banana']);
});
it('returns tags that have any word starting with the query string', () => {
assert.deepEqual(tags.filter('ar'), ['bar argon', 'argon']);
});
it('is case insensitive', () => {
assert.deepEqual(tags.filter('Ar'), ['bar', 'argon']);
assert.deepEqual(tags.filter('Ar'), ['bar argon', 'argon']);
});
it('limits tags when provided a limit value', () => {
assert.deepEqual(tags.filter('r', 1), ['bar']);
assert.deepEqual(tags.filter('r', 2), ['bar', 'future']);
assert.deepEqual(tags.filter('r', 3), ['bar', 'future', 'argon']);
assert.deepEqual(tags.filter('b', 1), ['bar']);
assert.deepEqual(tags.filter('b', 2), ['bar', 'bar argon']);
assert.deepEqual(tags.filter('b', 3), ['bar', 'bar argon', 'banana']);
});
});
......@@ -101,7 +115,14 @@ describe('sidebar.tags', () => {
}
const storedTagsList = fakeLocalStorage.getObject(TAGS_LIST_KEY);
assert.deepEqual(storedTagsList, ['foo', 'bar', 'future', 'argon']);
assert.deepEqual(storedTagsList, [
'foo',
'bar',
'banana',
'bar argon',
'future',
'argon',
]);
});
});
});
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment