Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Design Doc] In Doc Search UI #5707
[Design Doc] In Doc Search UI #5707
Changes from 16 commits
49016d7
229d371
072daa3
8f41b3a
aec1e97
e68e5fa
46c3218
0c6cbdf
0239240
34fc557
830a6bf
d28c748
aa1d828
097accb
ac95f50
726dc45
4718cc0
0fa000a
d532119
608c2b8
efda71f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if you put just
in-doc-search-ui-demo.gif
in both places here, Sphinx will just do the magic. Not sure, thoughThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tried it.
if we do this... then it is looking for the image in the same folder i.e.,
design/in-doc-search-ui.gif
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of note, supporting all Sphinx themes will also make it much easier for us to support mkdocs. A large part of the issue with around the UI/UX and theme integration, so we should hopefully be able to work around that now :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I'm about to say doesn't need to be added to this document but may be helpful to you more generally. Sometimes events like this are "debounced". In this instance, that means that the search query will only be fired every X number of milliseconds and only when the user stops typing. While you could use vanilla JS for this, there is a handy version in underscore already and underscore is already included by Sphinx (under the variable
$u
instead of the usual).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @davidfischer.
But what will be the benefit of debouncing the event?
will it not make our
suggestions loading time
look slower?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this would be a voting, I'd vote for this one :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also vote for that one. We are currently in this direction only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only concern with the extension is that it doesn't work locally. You can build your docs locally, but even if we hit the prod API, it will only return results from prod docs. This feels like a weird thing to do with an extension when it's inherently linked to our RTD production builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ericholscher
We have a global object
READTHEDOCS_DATA
and it containsapi_host
. In the extension, we are taking the API host from there.So in local, it is - http://127.0.0.1:8000
And we can use the extension locally (given that the Elasticsearch is correctly set up).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but that requires a running instance of RTD -- I mean it isn't useful outside of RTD. If you already have a local RTD instance, then bundling it with RTD would have the same outcome :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes... that is the limitation. 😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Split of code is a win to me. Easier to contribute, easier to focus when debugging, easier to keep up to date, etc. The downside is deploying a bugfix immediately, where core & extension need to be deployed together.
I understand that could feel weird that the results come from production while you are seeing a different set of docs. The extension could disable itself if detecting that it's running outside RTD if we can avoid that weird UX. Not a solution, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it will increase complexity in development environment..!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using one backend or the other does involve a lot of changes or it's just changing a setting and a class or similar? I want to know if this is something that we can test and see how it perform and decide after that or if testing this way is complicated since they are two different implementations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to have results like comment #1 then we have to do some changes in the backend and a new API endpoint. The changes may increase the size of elasticsearch index to x3 or x5. (In the demo, I did changes with my elasticsearch index locally to produce the results.)
Further changes can be.. like if we want to support section linking. Currently we just open the result page and user have to scroll down to the required section. For that... there will be considerable big changes. many parts of the elasticsearch will change -- how we index data/search data.
I don't think that we can test this without any changes.
Without any changes is okay too...the test-builds - https://readthedocs.org/projects/test-builds-dojutsu-user/versions/ are without any changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but the backend API should already be doing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. It is a question when we are changing/updating the backend.