feat: accessible search bar

[scissorsneedfoodtoo]

You can now focus on the search bar with 's' or '/', and use the up and down arrow keys to scroll through the hits. This also brings back the magnifying glass icon that, when clicked, either goes to the search page of the highlighted hit.

Closes #36693

[gitpod-io]

[scissorsneedfoodtoo]

Thank you for checking this out, @xyozio. I think you caught it while I was making some changes. Had to move some stuff around to add a bit more functionality. Please give it another look if you have a chance.

[raisedadead]

Hi @scissorsneedfoodtoo

Thanks for the PR, Looks good. But, I am wondering if this can quickly become unmanageable. How about we do dedicated handling of the hotkeys and this way we can extend this to other areas of the app such as in curriculum navigation (@ojeytonwilliams is working on some of those).

I found https://github.com/greena13/react-hotkeys to be quite well documented, but there could be other similar libs.

Let me know what you think?

[scissorsneedfoodtoo]

@raisedadead, yes, that sounds good to me. Right now focusing on the search bar is handled by Algolia's <SearchBox> component, but everything else is a bit hacked together. I'll look into a library to handle the up and down arrows and possibly focusing the search bar.

[ojeytonwilliams]

I'll probably re-review if I get the time, but I figured a partial review was better than nothing! Anyway:

I can't put the second half of the suggestion in, sadly, but I think

// Allow react-hotkeys to work on the searchbar
configure({ ignoreTags: ['select', 'textarea'] });

Should appear just after the imports in SearchBar, since it only needs to happen once.

Also, I noticed:

when, after reloading the page, you start typing in the text box. I think SearchHits is the culprit, as it calls handleHits which sets state.

[scissorsneedfoodtoo]

@ojeytonwilliams, thank you for your detailed review. I'll go through and implement your suggestions as soon as possible.

[ojeytonwilliams]

Thanks for implementing those suggestions @scissorsneedfoodtoo! After another read through, I have a few more ideas. Let me know what you think, particularly if I'm being overly picky.

[scissorsneedfoodtoo]

@ojeytonwilliams, not overly picky at all. I don't have a whole lot of experience with React, so I appreciate you and @RandellDawson going through this. I'll take all the help and suggestions I can get!

[ojeytonwilliams]

Okay, cool, I'm glad it's helpful!

I have one general observation, though I don't think it should happen in this PR. It looks like there's quite a lot of direct manipulation of the DOM (assigning ids via lifecycle methods, querying for elements by class and so on). It might be a little easier to maintain if it's refactored into a more standard React style. A custom component with the id as an attribute for the former and passing around the elements as props for the latter . That sort of thing.

As I said, I don't want to delay this PR, but I'd be happy to help with the refactor after it's landed.

[scissorsneedfoodtoo]

@ojeytonwilliams, that sounds great. Any help with refactoring this to be more standard React would be greatly appreciated.

[raisedadead]

Remove unwanted changes to lock files

[scissorsneedfoodtoo]

Went back to the earlier implementation of passing hits from SearchHits.js to SearchBar.js so it's not querying the DOM from lifecycle methods. Shouldn't throw errors now that the handleHits function is inside the useEffect hook.

[ojeytonwilliams]

It's looking good now. The only thing that stuck out was that hitsLength doesn't seem to be necessary. Can we just use hits.length instead?

[ojeytonwilliams]

LGTM 👍

[ojeytonwilliams]

@scissorsneedfoodtoo it's looking great, nice work!

@raisedadead I'm happy for this to be merged, if you are.

[raisedadead]

Code looks good to me. We could use some tests, I will go ahead and merge this first and @scissorsneedfoodtoo you could add some tests.

[scissorsneedfoodtoo]

@ojeytonwilliams, thank you for reviewing this! I learned a lot along the way.

@raisedadead, I'll start writing tests for the search bar ASAP.