#2779 Dev edition: add back jump-to search #2822
Conversation
|
This is still a WIP, need to add CSS for responsiveness and some missing JavaScript features. |
search.js
Outdated
| @@ -0,0 +1,41 @@ | |||
| ;(function() { | |||
There was a problem hiding this comment.
Can you put this in dev/? Then we don't need an additional build step to copy it.
No preceding semicolon please. "use strict"; at the top of the file please.
search.js
Outdated
| */ | ||
|
|
||
| // Get elements | ||
| var d = document; |
search.js
Outdated
| function findSections(word, arr) { | ||
| return arr | ||
| .filter((item) => { | ||
| const regex = new RegExp(`${word.toLowerCase()}`, 'gi'); |
There was a problem hiding this comment.
Why toLowerCase if you're going to add the i flag anyway? Also, no need for the template string.
search.js
Outdated
| .then(data => jsonResponses = data) | ||
| .catch(err => console.error('Error loading search-index.json')); | ||
|
|
||
| function resultTpl(result) { |
source
Outdated
| <div id="search" w-nohtml> | ||
| <div id="query-container"> | ||
| <input name="query" type="search" placeholder="Search. Press '/'" autocomplete="off" id="query" | ||
| class="field-input-text"> |
There was a problem hiding this comment.
No need for this class, or the class below, I think
search.js
Outdated
| const regex = new RegExp(`${word.toLowerCase()}`, 'gi'); | ||
| return item.text.match(regex); | ||
| }) | ||
| .slice(0, 10) |
There was a problem hiding this comment.
Maybe factor out this constant (10) into the top of the file
search.js
Outdated
|
|
||
| function findSections(word, arr) { | ||
| return arr | ||
| .filter((item) => { |
There was a problem hiding this comment.
Although the functional style is nice, it does mean we'll iterate over the entire array when we only want 10 elements. It might be better to go with a for loop? Or maybe it doesn't matter. I'm just a little concerned about mobile jank.
There was a problem hiding this comment.
I think that for the amount of data we are iterating over (~480 results) we can use .filter just fine. Also, in my opinion, this way we can select better results looking into all results.
But I'm probably not thinking through all the constrains, so if a for loop is a better option, lets do it.
There was a problem hiding this comment.
Happy to stick with the functional style for now and then I can do some measurements to see if it's a real issue.
search.js
Outdated
| .map(value => resultTpl(value)); | ||
| } | ||
|
|
||
| query.addEventListener('keyup', (event) => { |
There was a problem hiding this comment.
A lot of code in https://github.com/benschwarz/developers.whatwg.org/blob/97ff943a8f5b8fe38f78e224b4b44b472ccefb57/javascript/master.js for clearing results and navigating the list seems to be missing. Should we add it, or do you think it's not necessary?
There was a problem hiding this comment.
Is not necessary, but it's an absolutely nice to have. I'm adding those missing features.
dev/styles.css
Outdated
| } | ||
|
|
||
| div#search input, | ||
| div#Search ol { |
dev/styles.css
Outdated
| position: absolute; | ||
| top: 0; | ||
| right: 0; | ||
| } |
There was a problem hiding this comment.
Some more potential inspiration at:
- https://github.com/benschwarz/developers.whatwg.org/blob/97ff943a8f5b8fe38f78e224b4b44b472ccefb57/sass/all.scss#L37
- https://github.com/benschwarz/developers.whatwg.org/blob/97ff943a8f5b8fe38f78e224b4b44b472ccefb57/sass/desktop.scss#L21
- https://github.com/benschwarz/developers.whatwg.org/blob/97ff943a8f5b8fe38f78e224b4b44b472ccefb57/sass/handheld.scss#L31
|
I think it's done. @domenic please review as I'm sure I missed something. Small demo: |
Part of fixing whatwg/html#2779. See also whatwg/html#2822 which uses this. In the future we plan to have Wattsi generate this, but for now it is a Python-based postprocessing step. This can be bypassed by using the new --no-post flag.
Closes whatwg#2779. This depends on whatwg/html-build#114. The styles and scripts are based on https://github.com/benschwarz/developers.whatwg.org/tree/97ff943a8f5b8fe38f78e224b4b44b472ccefb57, but adapted and modernized, with some bugs fixed and behaviors tweaked.
|
So currently it looks like our build server does not have a recent enough Python/pip/virtualenv to work with this, hmm. I've asked for an upgrade and will report back when it's done, and then we can merge this. |
Final steps for #2779