Use API V3 #132

stsewd · 2023-03-09T01:58:46Z

Added 2 new options: rtd_sphinx_search_default_filter and rtd_sphinx_search_filters.
The options are passed as a JSON object to be read by our JS code.
The default filter defaults to the current project and version (extracted from the RTD env vars)
Filters are implemented as check boxes, we only support one filter at a time (when a filter is selected, we uncheck all others)

How to test locally:

Run your local version of RTD
To allow cross site requests, add |/api/v3/search at https://github.com/readthedocs/readthedocs.org/blob/b574f4b0dad34ccba8595cee72246d3473f8a2e5/readthedocs/settings/base.py#L769-L781
Change the rtd_dummy_data.json file
- 'proxied_api_host': 'http://devthedocs.org'
- Set the project and version to a project that you have locally
Change the value of rtd_sphinx_search_default_filter = "project:superproject/latest" to the slug of a project you have locally
Build the docs:

cd docs
make clean html

Open the _build/index.html file.

showcase

showcase.webm

Closes #129
Closes #130
Closes #131

humitos · 2023-03-15T09:33:54Z

I haven't tested this, but I just skimmed the code thinking about readthedocs/readthedocs.org#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 · 2023-03-15T15:36:58Z

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 · 2023-03-15T15:39:00Z

sphinx_search/static/js/rtd_sphinx_search.js

-            Search by <a href="https://readthedocs.org/">Read the Docs</a> & <a href="https://readthedocs-sphinx-search.readthedocs.io/en/latest/">readthedocs-sphinx-search</a> \
-        </div>';
+const generateAndReturnInitialHtml = (filters) => {
+    let innerHTML = `


HTML is mostly the same, diff shows like all changed, but only the trailing \ was removed to just use a multine string. The new addition is everything inside <div class="search__filters">

ericholscher

This looks reasonable, but I haven't looked at the JS super heavily. I need to spend a bit more time looking over this, but the high-level seems reasonable?

We're looking at a bunch of other ways to pass data around in the readthedocs-client, but the JS approach here looks workable to start.

sphinx_search/static/js/rtd_sphinx_search.js

humitos

I think this is good to move forward. I didn't find anything where I would block this PR. I really want to get this PR merged so I can continue working on the js client.

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

humitos · 2023-03-23T17:09:10Z

.readthedocs.yaml

-    # sphinx-js isn't compatible with python 3.10.
-    # https://github.com/mozilla/sphinx-js/issues/186
-    python: "3.9"
+    python: "3.11"


Suggested change

python: "3.11"

python: "3"

sphinx_search/extension.py

sphinx_search/static/js/rtd_sphinx_search.js

humitos · 2023-03-23T17:20:13Z

sphinx_search/static/js/rtd_sphinx_search.js

+    const config = getConfig();
+    // Add filters below the search box if present.
+    if (filters.length > 0) {
+      let li = createDomNode("li", {"class": "search__filters__title"});


I don't like too much the pattern of "generating nodes on the fly" a lot and I'd prefer to use literal strings if possible. This way is a lot easier to read and follow the code.

If you mean building the strings manually, we need to be more cautious about user input, a better pattern can be just building the skeleton first, and then change that (#117 (comment))

sphinx_search/static/js/rtd_sphinx_search.js

stsewd · 2023-03-23T18:07:44Z

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 · 2023-03-23T23:15:12Z

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 · 2023-03-23T23:47:32Z

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.

- update styles - integrate under `/` I copied all the code from readthedocs/readthedocs-sphinx-search#132. However, there are lot of things that are not required for this stage. I removed some of them and make the integration nicer with the client we are creating. These changes are related to the HTML structure and the CSS style. The functionality is exactly the same. Related #19

humitos · 2023-03-27T15:18:45Z

Cool! Since only tests were left here, I started using the .js file from this repository in the new client. See readthedocs/addons#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.

stsewd added 2 commits March 8, 2023 20:57

Use API v3

f36ee3d

Make it less ugly

58408d1

stsewd force-pushed the use-api-v3 branch from 1e2c870 to 58408d1 Compare March 10, 2023 00:50

stsewd added 4 commits March 14, 2023 14:13

Use checkboxes and append query in the API call only

2623bc9

Minifi

733ed5d

Updates

55cf57c

Remove link to docs

e759bd7

stsewd marked this pull request as ready for review March 14, 2023 22:12

stsewd commented Mar 15, 2023

View reviewed changes

ericholscher reviewed Mar 16, 2023

View reviewed changes

sphinx_search/static/js/rtd_sphinx_search.js Show resolved Hide resolved

humitos approved these changes Mar 23, 2023

View reviewed changes

stsewd added 3 commits March 27, 2023 11:44

Fix tests

4b7a8f8

Fix tests

cbfd7f2

Fix tests

6f95428

stsewd merged commit ca36a15 into main Mar 27, 2023

stsewd deleted the use-api-v3 branch March 27, 2023 18:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use API V3 #132

Use API V3 #132

stsewd commented Mar 9, 2023 •

edited

humitos commented Mar 15, 2023

stsewd commented Mar 15, 2023

stsewd Mar 15, 2023

ericholscher left a comment

humitos left a comment

humitos Mar 23, 2023

humitos Mar 23, 2023

stsewd Mar 23, 2023

stsewd commented Mar 23, 2023

ericholscher commented Mar 23, 2023 •

edited

stsewd commented Mar 23, 2023

humitos commented Mar 27, 2023

Use API V3 #132

Use API V3 #132

Conversation

stsewd commented Mar 9, 2023 • edited

showcase

humitos commented Mar 15, 2023

stsewd commented Mar 15, 2023

stsewd Mar 15, 2023

Choose a reason for hiding this comment

ericholscher left a comment

Choose a reason for hiding this comment

humitos left a comment

Choose a reason for hiding this comment

humitos Mar 23, 2023

Choose a reason for hiding this comment

humitos Mar 23, 2023

Choose a reason for hiding this comment

stsewd Mar 23, 2023

Choose a reason for hiding this comment

stsewd commented Mar 23, 2023

ericholscher commented Mar 23, 2023 • edited

stsewd commented Mar 23, 2023

humitos commented Mar 27, 2023

stsewd commented Mar 9, 2023 •

edited

ericholscher commented Mar 23, 2023 •

edited