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 search-as-you-type (inline search results) feature #2093

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

kaycebasques
Copy link
Contributor

Fixes #1977

@choldgraf
Copy link
Collaborator

This is super cool

@12rambau
Copy link
Collaborator

12rambau commented Jan 7, 2025

For me it's the most wanted feature, brilliant !

Copy link
Collaborator

@drammock drammock left a comment

Choose a reason for hiding this comment

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

thank you a thousand times for including such clear and detailed code comments. So far I've only read the implementation, haven't played around with it on the PR build yet.

Comment on lines +353 to +356
// Don't interfere with the default search UX on /search.html.
if (window.location.pathname.endsWith("/search.html")) {
return;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should also account for dirhtml builds, which I think (?) will have a url like https://whatever.com/en/search/ or potentially https://whatever.com/en/search/index.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dirhtml was not on my radar. Will need to look into what that does to figure out how it affects the impl.

// searchtools.js loads.
//
// Search class is defined in upstream Sphinx:
// https://github.com/sphinx-doc/sphinx/blob/master/sphinx/themes/basic/static/searchtools.js#L181
Copy link
Collaborator

Choose a reason for hiding this comment

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

link to .../blob/master/... may suffer from line number drift or otherwise go stale. Let's use a permalink to a specific repo state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SGTM

}

// Create a new search-as-you-type results container.
results = document.createElement("section");
Copy link
Collaborator

Choose a reason for hiding this comment

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

@gabalafou would appreciate your perspective on what is the best node type for an appearing/disappearing list of search results, and how this can/should/will interact with things like tab focus.

@@ -93,29 +93,50 @@
z-index: $zindex-modal;
top: 30%;
left: 50%;
transform: translate(-50%, -50%);
transform: translate(-50%, -30%);
Copy link
Collaborator

Choose a reason for hiding this comment

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

this change is just to make room below the modal for the results right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct

Comment on lines +319 to +320
focused_element_expected_content = "1. Test of no sidebar"
assert focused_element_actual_content == focused_element_expected_content
Copy link
Collaborator

Choose a reason for hiding this comment

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

this seems slightly fragile in case we (1) change our website content in a way that changes order of search results, or (2) sphinx changes their searchtools.js in a way that changes order of search results. Maybe not much to be done about that though, and we'll just fix it if/when it breaks... thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I can make it more robust like this:

  1. Use a CSS selector like #search-results .search li:first-of-kind to get the first node in the results
  2. Get the textContent of the node
  3. Use that programmatically retrieved textContent as the expected value

Copy link

github-actions bot commented Jan 8, 2025

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  src/pydata_sphinx_theme
  __init__.py
Project Total  

This report was generated by python-coverage-comment-action

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.

Search-as-you-type UX built on top of built-in Sphinx search tools
4 participants