-
-
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
Search: API V3 #9625
Search: API V3 #9625
Conversation
We were generating the context of the serializer from outside, but we can do this from the serializer itself, this way we don't have to duplicate this logic.
e84b905
to
e145700
Compare
This should be ready, but we should first merge #9624, so I can fix the merge conflicts :/ |
@stsewd This feels close. You mentioned some width issues w/ the search box, do you have a link to a good screenshot of that? |
@ericholscher sorry, I thought I've attached screenshots in the PR description, looks like I didn't. Basically, the text hides behind the "Search" button, and it also takes a lot of space that would be welcome when searching more than one project. We should probably just replace it with an icon and have the search bar not overlap with the button (not sure how this implemented in the new templates) |
Yea, 👍 on either hiding the search box, or just moving it down to a new line. |
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 started doing a review but I noticed that it will take more time than I thought. It seems to be a big project. I'll come back to it with more time and focus.
project = serializers.SerializerMethodField() | ||
version = serializers.SerializerMethodField() |
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 creating a proper serializer for each of them instead of methods?
Example of this from APIv3
urls = VersionURLsSerializer(source='*') |
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.
The only one we can do this for is for the version, for the project we need to call get_project_alias
to get the alias. The objects these serializers deal with are ES result objects, not RTD objects, we don't have that field in the ES object.
@ericholscher I'm trying to fix the search bar, but looks like we are using it in other places as well (the same structure/css classes). Current stateMy small fixLooks like the problem is with the width being hardcoded ( readthedocs.org/media/css/core.css Line 375 in 5e870d4
So I tried to fix it setting the width to The overlap is reduced, but now all forms have that small overlap. Moving the button to a new line may require some more CSS... New templatesThe new templates won't/don't have this issue If my small fix is enough, let me know. But I'm not sure if it's worth investing more time in that with the new templates coming 🤔 |
@stsewd I don't think we need to invest much time in this. It seems fine as is, as well. It's not broken, so definitely don't invest much time here. |
@stsewd I'm pretty happy with where this PR is. Is there any backwards incompatible changes required here, or can we roll it out and roll it back if needed? |
One small detail that you may have an opinion on #9533 (comment). What's next?
|
@stsewd we added this PR to the "iteration 0" milestone of the Diátaxis changes, so right now, I would hope that it's fixed up before Iteration 1 👍 |
@benjaoming not sure what you mean with this p: should be fine to merge as is? |
Yes absolutely, I wanted to clarify the below question
The answer is basically no, please merge, and that even resolves one of the items in the Diátaxis milestone 👍 🎉 |
@stsewd What is this exactly? The indoc search that isn't using our extension?
I don't fully understand why our extension would use v2 instead of v3? |
opps, that was my mistake, I meant to say "Use api v3 in our sphinx search extension"
yes |
@stsewd I've added |
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 I'm OK merging this, and if I can get it tested on Monday that will be useful, but otherwise it feels safe to deploy. We can continue to iterate on it as we get real user feedback, but there's a bunch of improvements here that I'm excited about. 👍
Implementation of #9533
Fixes #9573
This is on top of #9624
At first, I started to find a way to share most of the code with API v2, but the code resulted hard to follow,
so I just did a clean start for the new API (well, almost, there are still a lot of things that are being re-used).
type=file
.📚 Documentation previews 📚
docs
): https://docs--9625.org.readthedocs.build/en/9625/dev
): https://dev--9625.org.readthedocs.build/en/9625/