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

Allow to enable server side search for MkDocs #6986

Merged
merged 16 commits into from Jun 4, 2020
Merged

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Apr 29, 2020

This adds a feature flag to allow us to disable server side search in general and one to test enabling server side search for MkDocs projects.

TODO: regenerate the static assets and tested it locally.

Needs readthedocs/readthedocs-sphinx-ext#85
Closes #1088

@stsewd stsewd added the PR: work in progress label Apr 29, 2020
@stsewd
Copy link
Member Author

stsewd commented Apr 29, 2020

Also, we should document server side search as a feature of RTD, and linking to https://docs.readthedocs.io/en/stable/guides/searching-with-readthedocs.html

@stsewd stsewd removed the PR: work in progress label Apr 29, 2020
@stsewd stsewd requested review from ericholscher and a team Apr 29, 2020
@stsewd
Copy link
Member Author

stsewd commented Apr 29, 2020

One downside is that users need to trigger a build after activating the flag to disable server side search. And also if users want to test server side search for mkdocs they need to trigger a build (this shouldn't be necessary when we ship this feature globally)

@stsewd
Copy link
Member Author

stsewd commented Apr 29, 2020

To avoid these things we could start injecting a custom js per project rather than globally (_/js/rtd-embed.js)

@stsewd
Copy link
Member Author

stsewd commented Apr 29, 2020

And here is a comparison of our server side search vs mkdocs search

#6937 (comment)

@ericholscher
Copy link
Member

ericholscher commented Apr 30, 2020

One downside is that users need to trigger a build after activating the flag to disable server side search. And also if users want to test server side search for mkdocs they need to trigger a build (this shouldn't be necessary when we ship this feature globally)

Can we not disable it on the server? We should be able to send it down with the footer data. Also with the Sphinx search, if the RTD search fails we fallback to mkdocs, so we should likely have similar, which would make it not an issue.

@stsewd
Copy link
Member Author

stsewd commented May 1, 2020

@ericholscher so, I just tested this... but there is a race condition between fetching the footer api and initializing the search. Initializing search is faster. So, we could trigger the search after the footer is fetched (I don't think we want this) or call the api again before searching to fetch the feature flags (probable the same as the option 1, is going to make things slow).

Let me know if you think of a better way or if I should revert to the previous implementation.

@ericholscher
Copy link
Member

ericholscher commented May 4, 2020

@stsewd I think the race condition is fine -- I don't think too many people will hit the search before the footer loads. I guess the issue is people landing on the search page itself?

@ericholscher
Copy link
Member

ericholscher commented May 4, 2020

@stsewd also, have you done a bit of QA on the current production mkdocs results?

@stsewd
Copy link
Member Author

stsewd commented May 4, 2020

@stsewd I think the race condition is fine -- I don't think too many people will hit the search before the footer loads. I guess the issue is people landing on the search page itself?

In my local testing the race condition was always present, so the feature flag was always ignored.

@stsewd also, have you done a bit of QA on the current production mkdocs results?

Yeah, those are from the screenshots #6937 (comment)

@ericholscher
Copy link
Member

ericholscher commented May 10, 2020

This still needs a bit more thought before we ship it. We should find a solution that works similarly for both sphinx & mkdocs, and use it in both places. It seems we haven't had issues w/ the ability to disable Sphinx search, so we should likely do the same thing as there?

@stsewd
Copy link
Member Author

stsewd commented May 11, 2020

It seems we haven't had issues w/ the ability to disable Sphinx search, so we should likely do the same thing as there?

To disable sphinx search we require users to define the data.features.docsearch_disabled attribute themselves, so it works after a rebuild as well.

@ericholscher
Copy link
Member

ericholscher commented May 27, 2020

@stsewd I'm fine with it matching what we're currently doing on the Sphinx side. I'm 👍 on shipping this behind a feature flag once we do that. Sorry for the confusion here.

Copy link
Member

@ericholscher ericholscher left a comment

I feel like we're struggling with this on readthedocs/readthedocs-sphinx-ext#88 as well. We end up with weird race conditions on the search UX because we can't disable the default search for the tool. I'm wondering if there is a better way to handle this, since this seems brittle and likely to cause weird behavior. I don't have a great idea on how to fix it though :/

@stsewd
Copy link
Member Author

stsewd commented May 29, 2020

So, Mkdocs doesn't highlight the span tag, but it does the mark tag. I got this with just a replace in js.

Screenshot_2020-05-28 MkDocs(1)
Screenshot_2020-05-28 Read the Docs MkDocs Test

I can leave that... Or make the server accept an additional param called tag, options would be span and mark for now.

Let me know which one sound good, If we go for 2 can do that in another PR of course

@stsewd stsewd requested a review from ericholscher Jun 1, 2020
Copy link
Member

@ericholscher ericholscher left a comment

I think a simple JS replace is fine. Since this is behind a feature flag, I'm 👍 on shipping it next week.

@stsewd
Copy link
Member Author

stsewd commented Jun 4, 2020

@ericholscher readthedocs/readthedocs-sphinx-ext#85 needs to be merged in order to be able to deactivate search in sphinx projects.

@stsewd stsewd merged commit a6b60be into master Jun 4, 2020
2 checks passed
@stsewd stsewd deleted the mkdocs-indocsearch branch Jun 4, 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