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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable highlighting for web search results #2007

Merged
merged 2 commits into from
Jan 7, 2022

Conversation

adeak
Copy link
Member

@adeak adeak commented Jan 7, 2022

I've always been bugged by the fact that sphinx search results automatically highlight the search hits on the target page. This usually looks like this:
Screenshot from 2022-01-07 21-34-44
Note the noisy highlights of each appearance of "plot", even in the left sidebar in unrelated words.

I've done some digging and it seems there's no way to disable this feature altogether, at least I couldn't find anything in the sphinx docs nor in the pydata-sphinx-theme docs. I did find a workaround in this Stack Overflow answer that suggests hacking the highlight colour to be transparent in CSS.

So this PR does exactly that by creating an auxiliary .css file (similarly to the one we already have for the copy button), and injects it in conf.py (the same way that we do for the copy button). The result is nice:
Screenshot from 2022-01-07 21-35-02
But note that this will still have the ?highlight=... parameter in the URL, it will merely be a no-op. This is not great but I prefer this to the working highlight feature.

Furthermore, I don't actually know what I'm doing here (at all!) so labelling this as review-critical 馃槄 And of course if you think we should keep the default highlighting (cc @pyvista/developers) then we can just close this.

@adeak adeak added documentation Anything related to the documentation/website review-critical For changes that might have serious implications - always good to get a second set of careful eyes. labels Jan 7, 2022
@akaszynski
Copy link
Member

Always thought it was annoying as well, but I didn't think much of it. Good idea to disable.

Implementation is a hack, but it's documented and I don't think we should worry about it being perfect.

@adeak
Copy link
Member Author

adeak commented Jan 7, 2022

I just noticed that I copied over a syntax error from the answer on SO... let me rebuild the docs to make sure it's still working. It'll take a while.

@akaszynski
Copy link
Member

It'll take a while.

I'd like to take some time this release to improve our doc build time. 30 minutes isn't great for CI/CD.

@codecov
Copy link

codecov bot commented Jan 7, 2022

Codecov Report

Merging #2007 (217ecd9) into main (1990b01) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2007   +/-   ##
=======================================
  Coverage   93.06%   93.06%           
=======================================
  Files          74       74           
  Lines       15527    15527           
=======================================
  Hits        14450    14450           
  Misses       1077     1077           

@larsoner
Copy link
Contributor

larsoner commented Jan 7, 2022

I'd like to take some time this release to improve our doc build time. 30 minutes isn't great for CI/CD.

FWIW in MNE-Python for a few years now we have only built on CircleCI (which builds our docs) the examples that have been modified, which makes the doc build much faster (< 10 min rather than ~2 hours for a full build). Then all main builds are full. (Or at least they used to be -- nowadays we actually just do nightly ones.) It might be an option here, too. It ends up working pretty well because you can ask people to either make minor changes to examples expected to change, or push a commit with [circle full] to build all examples.

Code here if interested:

@adeak
Copy link
Member Author

adeak commented Jan 7, 2022

It'll take a while.

I'd like to take some time this release to improve our doc build time. 30 minutes isn't great for CI/CD.

Oooh that would be nice :) You have my full support!


The workaround still seems to work after rebuilding with the CSS syntax fixed. Developer tools shows no warnings related to that part of the CSS, and the style editor shows that the default colour is being overridden by our CSS snippet:
Screenshot from 2022-01-07 21-54-43

So I think it's working. Hopefully without subtly breaking anything else.

Copy link
Member

@akaszynski akaszynski left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@akaszynski akaszynski merged commit 5abc9ba into pyvista:main Jan 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Anything related to the documentation/website review-critical For changes that might have serious implications - always good to get a second set of careful eyes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants