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

Remove 'highlight' URL param from search results #6087

Merged

Conversation

@dojutsu-user
Copy link
Member

@dojutsu-user dojutsu-user commented Aug 21, 2019

No description provided.

@dojutsu-user dojutsu-user added this to In progress in In-doc search UI via automation Aug 21, 2019
@humitos
Copy link
Member

@humitos humitos commented Aug 21, 2019

I'm lacking context to review this PR. Why are we doing this?

@humitos
Copy link
Member

@humitos humitos commented Aug 21, 2019

Gotcha! I think the problem to solve here is different. Instead of removing the highlighting, I think we should make to highlight only the full text searched. In your example, Point media files at our media server.

@dojutsu-user
Copy link
Member Author

@dojutsu-user dojutsu-user commented Aug 21, 2019

@humitos

highlight only the full text searched

How can we achieve this?
I think, currently we are using default highlighting mechanism provided by Sphinx.

@humitos
Copy link
Member

@humitos humitos commented Aug 21, 2019

I have no idea 😁 --maybe solving it upstream? is it even reported?

My point is that we are removing a good feature because of a small bug in only one case and I think we need to solve that bug instead.

@stsewd
Copy link
Member

@stsewd stsewd commented Aug 21, 2019

@humitos there was a conversation in slack about this. Other suggestion was to have an ui element to deactivate the highlight. Also, I think that now we link to specific sections, is more easy to find the word you were looking for.

Personally I always modify the url to remove the highlight, and just use the firefox search if I need something like that

stsewd
stsewd approved these changes Aug 21, 2019
Copy link
Member

@stsewd stsewd left a comment

I'm +1 on dropping this

@stsewd
Copy link
Member

@stsewd stsewd commented Aug 21, 2019

@dojutsu-user dojutsu-user requested a review from ericholscher Aug 21, 2019
@ericholscher
Copy link
Member

@ericholscher ericholscher commented Aug 21, 2019

I think there are some theme updates we can do to make it easier to remove the highlighting, some patches to Sphinx in general, but I also always remove it when I hit the page. I think it should likely be opt-in with default off, which would make it much more user-friendly IMO.

@humitos
Copy link
Member

@humitos humitos commented Aug 22, 2019

OK. I like the highlight feature. I understand that there are cases where it's broken and I'm fine with this immediate solution for this.

I think the right solution is to fix these cases instead (I don't know how much work it means, though) and make this opt-in.

@dojutsu-user
Copy link
Member Author

@dojutsu-user dojutsu-user commented Aug 23, 2019

@ericholscher

I think it should likely be opt-in with default off, which would make it much more user-friendly IMO.

I remembered that I saw this feature in requests docs.

Peek 2019-08-23 12-07

Link: http://2.python-requests.org/en/master/user/quickstart/?highlight=payload#passing-parameters-in-urls

@ericholscher ericholscher merged commit 46be597 into readthedocs:master Sep 9, 2019
0 of 2 checks passed
@dojutsu-user dojutsu-user deleted the remove-highlight-url-params branch Sep 10, 2019
@dojutsu-user
Copy link
Member Author

@dojutsu-user dojutsu-user commented Sep 10, 2019

@dojutsu-user dojutsu-user moved this from In progress to Done in In-doc search UI Oct 3, 2019
@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Oct 18, 2019

I am a strong -1 on this change, I think we should revert this and figure out a better way to address whatever problems we were having with the results. Highlighting in doc is 100% a useful feature, though I always have to remove the highlight=... from the URL when linking to a user. This PR is lacking context as to why this change was needed or why we made this change, so I'd like to revisit this and consider reverting.

@dojutsu-user
Copy link
Member Author

@dojutsu-user dojutsu-user commented Oct 19, 2019

@agjohnson
I apologize for not writing the underlying reason and need for this PR.
The main reason for removal of highlight param was that highlighted keywords were mostly seems like distractions, like in this case: https://sphinx-notfound-page.readthedocs.io/en/latest/autoapi/notfound/extension/index.html?highlight=Point%20media%20files%20at%20our%20media%20server#notfound.extension.finalize_media

I always find it removing from the url.

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Oct 19, 2019

I raised some better fixes for this case in #6305

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants