Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for Mkdocs search #6937

Merged
merged 18 commits into from Apr 29, 2020
Merged

Add support for Mkdocs search #6937

merged 18 commits into from Apr 29, 2020

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Apr 21, 2020

Closes #1088
Related to #4588

Things to do:

  • Override the search js with ours to call ES
  • Cache the processed index file per project
  • Add tests

But currently is working if someone wants to test it :)

Screenshot_2020-04-20 Search manually Read the Docs

Screenshot_2020-04-22 Read the Docs MkDocs Test(1)

Screenshot_2020-04-22 Read the Docs MkDocs Test

@stsewd stsewd added PR: work in progress Needed: tests labels Apr 21, 2020
@stsewd stsewd removed Needed: tests PR: work in progress labels Apr 22, 2020
@stsewd
Copy link
Member Author

stsewd commented Apr 22, 2020

Some comparisons (letft RTD search, right MkDocs search).
Screenshot from 2020-04-22 17-24-21
Screenshot from 2020-04-22 17-23-15
Screenshot from 2020-04-22 17-22-16
Screenshot from 2020-04-22 17-21-33
Screenshot from 2020-04-22 17-20-58
Screenshot from 2020-04-22 17-20-40
Screenshot from 2020-04-22 17-19-35

@stsewd
Copy link
Member Author

stsewd commented Apr 22, 2020

To make the search available to all projects, we need to trigger a build or crawl all mkdocs projects (and versions) on storage and copy it's search/search_index.json file to the search media, after that we can trigger a re-index for those projects and all projects will start to use our search :)

@stsewd stsewd requested a review from Apr 22, 2020
@ericholscher
Copy link
Member

ericholscher commented Apr 27, 2020

@stsewd I wonder if it makes more sense to just read the search_index.json from the HTML output for mkdocs projects?

Copy link
Member

@ericholscher ericholscher left a comment

This looks like a good change. I'd like to:

  • Remove the json builder, so we can automatically reindex all existing mkdocs projects. It isn't needed, I don't think?
  • Start by indexing the files first, so that our site search works, but we don't override the in-doc search results quite yet. This will let us roll this out in a nice staged way instead of breaking all mkdocs projects.
  • Probably add a way to opt out of our search for mkdocs users as well

function init() {
var data = rtddata.get();
attach_elastic_search_query(data);
if (data.builder === 'mkdocs') {
attach_elastic_search_query_mkdocs(data);
Copy link
Member

@ericholscher ericholscher Apr 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This scares me a little bit, overwriting the users search content. Can we ship the indexing code and let it settle in before we try overriding the actual results for users?

Copy link
Member Author

@stsewd stsewd Apr 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I can revert the js change and put it in another PR respecting the "skip search" flag.

readthedocs/projects/models.py Outdated Show resolved Hide resolved
@stsewd
Copy link
Member Author

stsewd commented Apr 28, 2020

Changed to index directly from the html artifacts in storage and reverted the js changes, this is ready for review.

@stsewd stsewd requested a review from ericholscher Apr 28, 2020
Copy link
Member

@ericholscher ericholscher left a comment

I'm 👍 on this change. It should let us start indexing data and make site search work, and we can then integrate it into the docs once we confirm that all works. 🎉

@stsewd stsewd merged commit 9802ad0 into master Apr 29, 2020
2 checks passed
@stsewd stsewd deleted the mkdocs-search branch Apr 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants