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

Add #searchbox to sidebar for "Hide Search Matches" link #925

Merged
merged 2 commits into from Sep 13, 2022

Conversation

minrk
Copy link
Contributor

@minrk minrk commented Sep 12, 2022

The #searchbox element is part of the base sphinx theme, and creates a link to remove ?highlight=... from the URL. Usually this is part of the search box in the sidebar, but this theme doesn't use sidebar search, so the "Hide Search Matches" link doesn't have anywhere to go.

added to right sidebar, but I'm not 100% sure that's the best place for it:

hide-matches

It's more obvious where this should go in themes that have a sidebar search box, like Furo:

Screen Shot 2022-09-12 at 11 02 03

Alternative: custom UI / javascript to remove ?highlight=... from the URL rather than inheriting. It should not be complex to reimplement if this approach doesn't suit the theme.

part of the base sphinx theme,
creates a link to remove ?highlight=... from the URL

added to sidebar
Copy link
Collaborator

@12rambau 12rambau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know this functionality of Sphinx ! I like the way you implemented it so I'm +1 for this modification

Copy link
Collaborator

@12rambau 12rambau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you need to add the new searchbox.html in the list here as well: https://github.com/pydata/pydata-sphinx-theme/blob/main/docs/user_guide/layout.rst#L437

Copy link
Collaborator

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had no idea this existed either, but cool! +1 once @12rambau 's comment is addressed

@minrk
Copy link
Contributor Author

minrk commented Sep 13, 2022

Added to the layout list.

TDD (Twitter Driven Development) FTW! Thanks @asmeurer for the pointer.

@asmeurer
Copy link
Member

As long as we're thinking about this, is it possible to just remove the search term from the URL entirely? I personally see no value in having it there.

@choldgraf
Copy link
Collaborator

re: disable the search entirely, I'm not sure if that's possible in Sphinx or not - maybe we can track that in a follow-up issue? Unless there is quick agreement that this is possible and people want it, it might require more discussion and investigation.

@12rambau
Copy link
Collaborator

As long as we're thinking about this, is it possible to just remove the search term from the URL entirely? I personally see no value in having it there.

If you try the build from this PR and check this page: https://pydata-sphinx-theme--925.org.readthedocs.build/en/925/examples/example_pandas.html?highlight=quick+start, clicking on the button not only make the target disapear from the page but also change the URL.

@asmeurer is it what you requested ?

@asmeurer
Copy link
Member

No I mean never have it there at all (but still highlight the matches). The only point of having it in the URL is to keep it intact when copy pasting it, but generally when you copy the URL of a page you don't want the highlights dragged along.

My knowledge of JavaScript isn't great, but isn't it possible to remove the query parameter from the URL but keep the highlights? Obviously it wouldn't survive a refresh, but I think that's fine.

@12rambau
Copy link
Collaborator

ok then that's a problem that needs to be handled from Sphinx side as we rely on it for the whole search system. I think it would not be wised to mess with it in our theme and risking incompatibilities with other sphinx extensions that also rely on this mechanic.

I'll merge this PR and let you move the issue to the sphiinx repository: https://github.com/sphinx-doc/sphinx

@12rambau 12rambau merged commit d80e052 into pydata:main Sep 13, 2022
@choldgraf
Copy link
Collaborator

Just a note that it seems what @asmeurer suggests might be possible without changing the behavior of sphinx or the page itself:

https://developer.mozilla.org/en-US/docs/Web/API/History/replaceState

That seems to suggest you can only remove the highlight= section from the page URL after the page loads, so if people copypaste it will not be there.

@12rambau
Copy link
Collaborator

12rambau commented Sep 13, 2022

Sure but again are we sure nothing is using it ? As we can't guarantee it I think it's safer to let it as it is from our side and leave it to the advanced developer to add this small JS patch if they want

@asmeurer
Copy link
Member

Sure but again are we sure nothing is using it ?

The suggested change wouldn't break existing URLs with the highlight query parameter. It would only remove it from the URL bar so it doesn't persist into copied URLs. I've seen Google search results with the ?highlight= parameter in them, because apparently even Google was tricked by these copied URLs. I suspect a lot of people don't even realize it is there. And even if you know about it, it's annoying to clear the query parameter, and to remember to clear it, before copying a URL.

As we can't guarantee it I think it's safer to let it as it is from our side and leave it to the advanced developer to add this small JS patch if they want

The number of people who are annoyed by this and the number of people who know enough Javascript to fix it are a very small intersection (include me as one of the people who doesn't know how to do it). If you're worried about this, a better option is to implement it but make it configurable.

Note that I'm not necessarily saying this needs to be fixed in the theme. Maybe it makes more sense to fix it upstream in Sphinx. I only mentioned it here because it seemed like a relevant place to bring up the discussion.

@12rambau
Copy link
Collaborator

As mention earlier I think you should open an issue on sphinx directly and reference this PR, they will know better than us the consequences of such a change

@asmeurer
Copy link
Member

I opened an upstream issue in Sphinx here sphinx-doc/sphinx#10833.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants