-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Conversation
Thanks for the design doc. I think we need a hybrid solution here which is mixed of |
@safwanrahman Also, some problems might be associated with completion suggester + edge_ngrams
|
Firstly, that short demo looks very cool. This design doc jumps right into specific implementations of how this will work in Elastic Search and while that's pretty important I think we are missing a few higher level things. For example, there's two parts of the in-doc search UI -- the normal search results page and a new search as you type instant-answer overlay. Are you planning to tackle any improvements to the normal Sphinx search page or exclusively tackling the overlay? Also, how customizable do we want to make this?
I went through a lot of things here and we don't need to do all of these things for GSoC! There probably isn't enough time. I probably missed some things too! However, I want to get thinking about how this will fit into the codeline, how we'll roll it out, and how project maintainers will be able to customize things. I don't want to focus exclusively on implementation details. |
Thank you.
Linking some PRs on Algolia integration with the docs:
Thanks @davidfischer for the points. Those were some important ones which i didn't consider in the first place. I am updating the Design Docs to include those.
I don't have answers to these questions yet. I will do some research on these. |
@davidfischer For opt-in/out, we basically need a javascript bool type variable and from that we can wrap our code in # conf.py
...
RTD_AUTOCOMPLETE = True
... Then we can make changes in our readthedocs-sphinx-ext to check for this variable and if this is true, set a javasciript variable in the template to true, like var RTD_AUTCOMPLETE = true; This can be done in this way - https://github.com/thewtex/sphinx-contrib/blob/master/googleanalytics/sphinxcontrib/googleanalytics.py and we got our variable which is globally available and we can use it to control this feature. And for the styles, they can have a custom css file embed in their docs (which can be configured from conf.py) like this (tested with short demo) - https://github.com/rtfd/readthedocs.org/blob/f12f5fd4adae415959caf56323ed0eeb77103ae7/docs/conf.py#L107-L108 Also for rolling out this feature, after testing this with our docs, can we ask some other project maintainers who are currently using RTD to enable this in their project and provide some feedback on this? |
I think if we add the ability to disable our search (falling back to Sphinx's built-in search) or to search subprojects, we should probably add some messaging like "search provided by Read the Docs". We could even have a link to click to get the built-in search results. We also need a way in the UI to show if a search result came from a subproject or something like that.
We don't currently. However, I could see some useful ones like excluding subprojects, looking for certain terms in titles only vs anywhere in the body, or something like that.
One way that would involve a significant refactor would be to expose all the functionality we do on in-doc pages via a JavaScript global variable This could be used for:
Thoughts? |
I am +1 on the message. But, how will showing the built in results benefit?
It is great idea. We should consider this. This will increase the complexity though.
We can do this. I don't know if it's going to increase the complexity or not. Also, I would like to know your thoughts on my comment above. One thing that we can also provide is the search analytics to the project maintainers (who are using our search)? I don't know if it is required or not. But it will be a great feature. They can just go to the admin dashboard and see the analytics. I think we are tracking many things here and we should prioritize them before starting the work. Like, search analytics, if agreed upon, might me the last on our to-do list. |
I think we don't use the |
Thanks. This is a valid point. YAML file makes more sense. PS. Closed the PR by mistake. Opened it again though. |
Agreed. I want to focus this a bit more than the current discussion. My first question is:
Once we answer that question, we can break the work down into pieces. My goal is to focus mostly on the front-end here, at least to start, since that's the area where we historically have needed more work. Once we get a really good UX around search, then we can work on improving indexing and other backend related things to make the front-end function better. |
(Sorry, didn't mean to close it, wrong button! :) |
@ericholscher One problem I faced is how to include CSS file for this. I was trying to include it in the same way as we include
What do you mean 'technology' here? cc: @davidfischer |
Wow, the demo is really good. Just a comment here, since I suppose that the UI has not been discussed yet, but to consider in that case. If we are going to show these kind of results as you type, I'd suggest to open a modal once the user clicks on the "Search" box input, as Slack does. It takes control of the screen over the page that you are seeing it suggest you things, but if you don't find what you are looking for from the suggested ones, you hit enter and you get more specific results in the same modal So, this way we can differentiate our own search from the default search provided by Sphinx and both can live together. |
@humitos |
@humitos This is just a demo and it will much better than this. Like backdrop and other things. |
I am thinking to start the work. What are your thoughts? |
I'm good focusing on in-doc modal search. That means that we are mostly ignoring the built-in Sphinx search page for the purpose of GSoC. I'm fine 👍 with that. Basically, that limits the scope to:
I think that's probably a good idea to scope this down for the purpose of GSoC. We have a clear, somewhat self-contained project. There's already enough complexity here.
I think he means like any libraries we will need to do this. For example, we try to support all the way down to IE11 with our docs theme and Read the Docs generally and I think we want to support that here. For example, that means you can't use the JavaScript
Currently, the CSS included for in-doc search is here: https://github.com/rtfd/readthedocs.org/blob/master/media/css/readthedocs-doc-embed.css Currently, we inject our CSS and JS into the Sphinx build process here: https://github.com/rtfd/readthedocs-sphinx-ext/blob/50e43731311006cb51a530bc34df89386de8bcf6/readthedocs_ext/readthedocs.py#L53 You have a few options.
With respect to @humitos comments around a modal window versus an overlay, I don't have a particularly strong opinion. I do hope that we can change the UI relatively easily and I don't want to have to make the decision between modal and overlay quite yet. It might be nice to roll it out to a subset of users and begin to get some real user feedback before committing to a specific UI/UX. |
With respect to concrete feedback on the document in this PR, I think there's a few bits that need to be moved around and a few bits that need to be added. A few things that we should probably add:
Here's a short guide on writing design documents if that's helpful. HeadingsLooking at just the headings of the doc in this PR, I think we could have something like this:
|
@davidfischer
I don't think so, I tried that with the demo #2 and the results were not very good. We get the suggestion after typing the whole word. Like.. It will not give any result for Also, Thanks for a great advice on the design docs. I have restructured the whole docs according to it (I have copied few lines from it). |
I was trying via sphinx-extension method (mentioned by @davidfischer in above comment). The main problem is that we can't use The problem it poses is that I think we might not want to that much hack around it (or we want?). Or else we can use options 1 or 2 from the comment above. Edit: We can use DOMContentLoaded event (I totallly forget about this) and can have access to |
This is what you want! Support is good for all the browsers we care about.
I'd start with a new repo and we can change that later if necessary. |
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 the document is very close to being ready to merge. I don't consider it a final document and we can always update it as we make more design decisions.
One thing I would add is a note at the top (like this)mentioning that this is not yet a live feature and it is in active development. That way if a user stumbles across it they know they can't use it yet.
We plan to select the search bar, which is present in every documentation, | ||
using the `querySelector()`_ method of JavaScript. | ||
Then add an event listener to it to listen for the changes and | ||
fire a search query to our backend as soon as there is any change. |
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?
Co-Authored-By: David Fischer <[email protected]>
Co-Authored-By: David Fischer <[email protected]>
Co-Authored-By: David Fischer <[email protected]>
Co-Authored-By: David Fischer <[email protected]>
@davidfischer |
|
||
.. figure:: ../_static/images/design-docs/in-doc-search-ui/in-doc-search-ui-demo.gif | ||
:align: center | ||
:target: ../_static/images/design-docs/in-doc-search-ui/in-doc-search-ui-demo.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.
I think if you put just in-doc-search-ui-demo.gif
in both places here, Sphinx will just do the magic. Not sure, 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 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
.
* Support across all (or virtually all) Sphinx themes. | ||
* Support for the JavaScript user experience down to IE11 or graceful degradation where we can't support it. | ||
* Project maintainers should have a way to opt-in/opt-out of this feature. | ||
* (Optional) Project maintainers should have the flexibility to change some of the styles using custom CSS and JS files. |
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'm not sure if this is something where we need to do something. If we start the project by adding a specific HTML class to all the new elements that we add in the UI, it should be possible to change the style by the user without us implementing something in particular. Maybe I'm missing something, 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.
Yes I agree.
I think I should remove this point??
I am guessing that user won't be overwriting JS functions ... !!
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.
We need to document how users can override it, so I'm 👍 on keeping it.
and it will get included. | ||
* Package the in-doc search into it's own self-contained CSS and JS files | ||
and include them in a similar manner to `readthedocs-doc-embed.*`. | ||
* It might be possible to package up the in-doc CSS/JS as a sphinx extension. |
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 contains api_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..!!
+++++++ | ||
|
||
We have a few options to support `search as you type` feature, | ||
but we need to decide that which option would be best for our use-case. |
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.
I believe this document is mostly ready. I'd be 👍 on merging it soon, and then we can update it with PR's as we go and make implementation decisions, but I think it's at a good place for now.
++++++++++ | ||
|
||
* For the initial release, we are targeting only Sphinx documentations | ||
as we don't index MkDocs documentations to our Elasticsearch index. |
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 :)
++++++++++++++ | ||
|
||
* Should we rely on jQuery, any third party library or pure vanilla JavaScript? | ||
* Are the subprojects to be searched? |
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.
Merging this, we can make any updates to this doc in an additional PR. |
Initial design doc for In Doc UI.
Closes #5699