-
Notifications
You must be signed in to change notification settings - Fork 189
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 improved search using rtd-sphinx-search #4111
Conversation
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 looks great, I'm just a bit worried about the dependency.
01f619f
to
e08b03f
Compare
I rebased the branch and updated the base branch to be |
The real thing that is still blocking the search from being useful is still that the results aren't particularly useful. The API reference dominates the search results, and I think this is rarely what a user is after. I have been googling for quite a while now to see if we can configure or customize the search behavior and maybe penalize the API reference, but so far no dice. |
Yeah, something like this: readthedocs/readthedocs.org#7082 |
e08b03f
to
af2e16c
Compare
af2e16c
to
fd3d7c2
Compare
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.
Looks like there is some issue with the dependencies?
utils/dependency_management.py
Outdated
install_requirements.update({Requirement.parse(r) for r in setup_cfg['extras_require'][key]}) | ||
|
||
# add requirement for "search as you type" | ||
install_requirements.update({Requirement.parse('readthedocs-sphinx-search~=0.1.0')}) |
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.
Why not add this to the docs
extra? I know it only makes sense on RTD, but can't hurt to install it, no?
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.
Well... I would say things I don't need hurt my disk ;-)
I'm not strongly against including it, if others think this is better
includes "search as you type" https://github.com/readthedocs/readthedocs-sphinx-search Co-authored-by: Sebastiaan Huber <[email protected]>
18a6bb3
to
abd7004
Compare
Codecov Report
@@ Coverage Diff @@
## develop #4111 +/- ##
===========================================
+ Coverage 78.89% 78.89% +0.01%
===========================================
Files 467 467
Lines 34481 34481
===========================================
+ Hits 27199 27202 +3
+ Misses 7282 7279 -3
Continue to review full report at Codecov.
|
@csadorf thanks for the note, this came from the rebase (fixed now).
Is this a known issue? |
Haven't ever seen that one before, but it seems that an illegal instruction is executed on a different thread, which almost certainly that of the RMQ communicator. Not sure why this would pop up now. We have been refactoring |
Ok. For the record, this is an example of a failing CI run: And the error message is:
Indeed this is using a rather recent release of pymatgen: |
I think "Illegal instruction" means it's trying to execute a CPU instruction that is not supported. My guess would be the Here's a related And an old issue from |
Update on the failing test: should be fixed, |
Thanks @greschd ! Problems that solve themselves are the nicest problems :-) Ready for re-review! |
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.
LGTM! :)
Ok, then let's give this a try - let's see how people react ;-) |
@ltalirz ermm my reaction is that its faulty lol; the search box appears behind the builtin one, so you can't actually see what you are typing: (perhaps some custom CSS can sort this out) |
For me it works. Maybe it is due to your dark theme CSS? Can you try the default one? |
Also of note is that it fixes itself when the screen gets below a certain width (i.e. via the CSS @media rules) |
Jup, can confirm having the same issue - tested in Firefox and Edge. Works in Internet Explorer though 🤣 |
Also, in my Firefox, when I type enter, I then can't correct (modify) the word I'm searching. I'm forced to write it back fully. Also, I can't paste there anything. |
That actually seems like a more serious UX issue - I'd vote to revert for now, until we can fix some of these issues. |
I'm on firefox on Ubuntu 18.04 and seems to work fine on a 4k screen so width as a cause seems unlikely |
anyway I have opened a issue: pydata/pydata-sphinx-theme#202, so I would vote to revert for now and monitor the progress of that issue |
Should we also open issues for the paste / edit problems on |
If someone else experiences the same problem, yes. Do you @greschd ? |
What I meant was that for me it does not seem to depend on the width, using the same browser, so it cannot be exclusively a width problem |
@bosonie I can paste, but when I try to edit a previous search the search string is in the "wrong" search bar. |
Regarding the other issues, yeah the UI is quite flaky. If you search and it finds some results, you can no longer edit it. There is the cross mark in the top right, which you might think resets the search bar, but in fact it just closes the window |
Ok, I review the "paste" problem and probably I had a problem with my keyboard. Now Ctrl+V works, but still I can't paste with the central button of the mouse, a feature that was present in the old system. Moreover, you can't copy from the search bar below, therefore you can't copy your old searched word. |
If this produces more issues than it solves, I'm perfectly fine with reverting this. |
Hm... I will prepare it myself - I think some of the changes in the validation script should remain |
See #4180 |
thanks @ltalirz, I really like the UX (when working), so hopefully we can get it fixed soon and add it back in 🤞 |
includes "search as you type" using readthedocs-sphinx-search
(see features )
Try it live at https://aiida.readthedocs.io/projects/aiida-core/en/rtd-elasticsearch/