readthedocs / readthedocs-sphinx-search

Deprecated: Enable search-as-you-type feature for docs hosted by RTD.
https://readthedocs-sphinx-search.readthedocs.io/
MIT License
33 stars 16 forks source link

Use API V3 #132

Closed stsewd closed 1 year ago

stsewd commented 1 year ago

How to test locally:

cd docs
make clean html

showcase

showcase.webm

Closes https://github.com/readthedocs/readthedocs-sphinx-search/issues/129 Closes https://github.com/readthedocs/readthedocs-sphinx-search/issues/130 Closes https://github.com/readthedocs/readthedocs-sphinx-search/issues/131

humitos commented 1 year ago

I haven't tested this, but I just skimmed the code thinking about https://github.com/readthedocs/readthedocs.org/pull/10127 and how we are going to integrate it there. It seems that the only relevant change is the addition of two new configs. We can get those from the user (.readthedocs.yaml) or the doctool itself (readthedocs-build.yaml). I think it looks good and we will be able to integrate it properly 👍🏼

Filters are implemented as check boxes, we only support one filter at a time (when a filter is selected, we uncheck all others)

Instead of using check boxes, I'd say we should use radio buttons then, which provide this UX natively.

stsewd commented 1 year ago

Instead of using check boxes, I'd say we should use radio buttons then, which provide this UX natively.

Radio buttons don't allow you to uncheck them.

stsewd commented 1 year ago

are there specific things you want to double-check before merging?

Well, I need to fix the test :D, but wanted a review first, before dealing with selenium

ericholscher commented 1 year ago

I'm 👍 on shipping this, since it's not super heavily used, if it's been reviewed. I think user feedback will be the main thing that we care about. I agree this is a really exciting addition, so let's ship it and get it running on our docs, and then we'll get lots more feedback I imagine :)

If the tests are super brittle or annoying, we can also discuss changing them up in some fashion. I don't have a ton of context about how complex they are, but I feel like selenium is probably overkill, and we could be using some lighter weight JS test stuff? Though again, I don't have much knowledge there..

stsewd commented 1 year ago

If the tests are super brittle or annoying, we can also discuss changing them up in some fashion. I don't have a ton of context about how complex they are, but I feel like selenium is probably overkill, and we could be using some lighter weight JS test stuff? Though again, I don't have much knowledge there..

Yeah, I'd prefer more unit tests here at the js level. But we do have some UI logic that selenium helps to test, but also, it's small, so it isn't hard to manually test that.

humitos commented 1 year ago

Cool! Since only tests were left here, I started using the .js file from this repository in the new client. See https://github.com/readthedocs/readthedocs-client/issues/19. I had to modify some part of it to allow me to move forward with the new integration. However, I don't think that's a problem since it's already using the APIv3 backend -- which is the work done in this PR.