Uh oh!
There was an error while loading. Please reload this page.
Organize searchbar interface results by page - #126
Conversation
be75704 to
7957014Compare
marvinchin
left a comment
There was a problem hiding this comment.
Looks like a good change. Just a couple of comments for now!
| .filter(searchKeyword => searchKeyword !== '') | ||
| .map(searchKeyword => searchKeyword.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')) | ||
| .map(searchKeyword => new RegExp(searchKeyword, 'i')); | ||
| .map(searchKeyword => new RegExp(searchKeyword, 'ig')); |
There was a problem hiding this comment.
I guess if the aim is to find the total number of matches (rather than the total number of regexes it matches), it seems reasonable to have it. Is that the expected behaviour here?
| if (isMatchingPage) { | ||
| matches.push(Object.assign(entry, { totalMatches })); | ||
| } | ||
| const fallbackTitle = title || src; |
There was a problem hiding this comment.
The term fallback feels a little misleading here, since this variable would be used as the eventual title anyway. Would something like resultTitle be more accurate?
| const matchesKeywords = headingKeywords[id] && headingKeywords[id].some(keyword => | ||
| regexes.some(regex => regex.test(keyword))); | ||
| if (matchesHeading || matchesKeywords) { | ||
| searchTarget = [fallbackTitle, keywords, text, ...(headingKeywords[id] || [])].join(' '); |
There was a problem hiding this comment.
I think it might be helpful to add some comments to explain what we're trying to do when we are changing the search target here 🙂
There was a problem hiding this comment.
Using a more specific variable name might help too!
| keywords, | ||
| ...Object.values(headings), | ||
| ...Object.values(headingKeywords), | ||
| ].join(' '); |
There was a problem hiding this comment.
It feels like it would be nice to move the join(' ') part to getTotalMatches, so that we can just pass a list of search targets to the function instead of having to join it manually every time.
| <span class="page-title" v-html="highlight(item.title, value)"></span> | ||
| <br v-if="item.keywords" /> | ||
| <small v-if="item.keywords" v-html="highlight(item.keywords, value)"></small> | ||
| <hr class="page-headings-separator"/> |
There was a problem hiding this comment.
@ang-zeyu oops, I missed this just now. There seems to be an inconsistent spacing here between the class and the closing />
163b1aa to
a2e1daaCompare
Thanks for the suggestions! I've updated the relavant parts I've updated the testing instructions with a link to a version of
Yup, the old |
marvinchin
left a comment
There was a problem hiding this comment.
Thanks for making the changes! Just a couple of follow up comments.
I don't think that we should show the number of matches to the reader - that doesn't seem to add much value to the search experience and might be confusing.
| function getTotalMatches(searchTarget, regexes) { | ||
| return regexes.reduce((total, regex) => (regex.test(searchTarget) ? total + 1 : total), 0); | ||
| // Returns the total number of matches between an array of regex patterns and string search targets. | ||
| function getTotalMatches(regexes, ...searchTargets) { |
There was a problem hiding this comment.
Sorry if it wasn't clear before, but I was thinking of having a variable searchTargets. I don't feel that the spread operator gives us much benefit here, and makes it a little more confusing at the call site.
There was a problem hiding this comment.
Agreed, I'm still not so sure what you mean by "having a variable searchTargets" though;
As a guess, do you mean removing searchTarget/searchTargets as a parameter from getTotalMatches altogether, and using a variable local to primitiveData()?
There was a problem hiding this comment.
I was thinking something along these lines:
function getTotalMatches(regexes, searchTargets) {
...
}
const searchTargets = [...];
const totalMatches = getTotalMatches(regexes, searchTargets);
| } | ||
| const displayTitle = title || src; | ||
| // The total number of occurrences of all indexed words ( headings, keywords, title ) for a page |
There was a problem hiding this comment.
Just a nit, but should we stick to the usual spacing conventions here for the bracket? (i.e. (headings, keywords, title))
ang-zeyu
commented
Feb 6, 2020
Hmm, the PR's version shouldn't show the number of matches, only the template file I linked in the testing instructions, for verify the sorting algorithm. Can I confirm this? |
marvinchin
commented
Feb 6, 2020
Ah I think I misunderstood earlier that you were suggesting that we add the match counts to the production version. Yes, the number of matches don't appear in the search result in the PR version. Thanks for clarifying! |
Search results belonging to the same page display as individual entries, showing the page title multiple times. Results are also not organized by page, but by the total number of matches in the page title, heading and keywords with the search term. When a page title is not specified in the site config, the search result displays an empty block of vertical space. This can lead to a less pleasant user experience with the searchbar. Let’s redesign the searchbar user interface, organizing results by page. Results belonging to the same page are sorted by the number of matches, and pages are sorted according to the total number of its matches in the page’s headings and keywords. Let’s use the page’s src as a fallback title if the title is absent.
a2e1daa to
305bc02Compareang-zeyu
commented
Feb 7, 2020
Thanks for clarifying as well! I've updated it and removed the comments in favor of more explicit variable names |
ang-zeyu
commented
Feb 9, 2020
Thanks a lot for reviewing this! 😄 |
What is the purpose of this pull request? (put "X" next to an item, remove the rest)
• [x] Enhancement to an existing feature
ResolvesMarkBind/markbind#994
Requires MarkBind/markbind#1006
What is the rationale for this request?
Search results are not organized by page, resulting in a less pleasant user experience.
What changes did you make? (Give an overview)
searchbarTemplateto its own seperate file for better maintainability.searchbarTemplate's template to organize results by pageEnd result:
Provide some example code that this change will affect:
Template of
searchbarTemplateIs there anything you'd like reviewers to focus on?
na
Testing instructions:
The test site has a good amount of keywords / headings used to test the sorting algorithm.
This version of
SearchbarPageItemdisplays the number of matches beside the entry, if neededThe searchbar should appear as is in the above image otherwise.
Proposed commit message: (wrap lines at 72 characters)
Organize searchbar interface results by page
Search results belonging to the same page display as individual entries,
showing the page title multiple times.
Results are also not organized by page, but by the total number of
matches in the page title, heading and keywords with the search term.
When a page title is not specified in the site config, the search result
displays an empty block of vertical space.
This can lead to a less pleasant user experience with the searchbar.
Let’s redesign the searchbar user interface, organizing results by page.
Results belonging to the same page are sorted by the number of
matches, and pages are sorted according to the total number of its
matches in the page’s headings and keywords.
Let’s use the page’s src as a fallback title if the title is absent.