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

Search function no longer hangs #39

Merged
merged 1 commit into from
Aug 5, 2019
Merged

Search function no longer hangs #39

merged 1 commit into from
Aug 5, 2019

Conversation

hunse
Copy link
Collaborator

@hunse hunse commented Aug 2, 2019

Fixes #28. Based on the fix in sphinx-doc/sphinx#6394.

To test this locally, cd to the folder where you've built the docs with sphinx, and run python -m http.server (you need the HTTP requests to do the search properly).

I also tested this remotely by pushing the docs to a gh-pages branch in one of my own repos (https://hunse.github.io/docs-test/).

This raises the idea that we might want to somehow track changes in the Sphinx repository to their themes, in case we need to make those changes to our themes as well. I don't think it needs to be anything automated, but we could make it part of the release procedure or something, and also keep track somewhere when we last checked the Sphinx repository for new changes.

@tbekolay
Copy link
Member

tbekolay commented Aug 2, 2019

somehow track changes in the Sphinx repository to their themes

My initial thought would be to completely decouple our theme from the existing themes (we are built on top of the "basic" theme) but I don't think that would have helped us here unless we also copied in all of the JS / CSS in those themes, which would probably balloon the repository up pretty big. So that's probably not a good solution.

We could instead pin the version of Sphinx that we use, and as part of the Nengo Sphinx Theme release process see if there's an updated version of Sphinx and upgrade to it as part of the release. For this, we would have to have a short list of things that we check are working correctly, as I definitely would never think to use the search box on my own.

I don't think I would prioritize it really, but if you think it's worth doing then feel free to split off a new issue @hunse.

Copy link
Member

@tbekolay tbekolay left a comment

Choose a reason for hiding this comment

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

I'm not 100% sure, but I think the Fixes #28 part has to be in a new paragraph at the end of the commit message ... maybe? That's how we've done it in all other commit messages I can remember, anyhow.

I'll merge this in once the Black PR is merged in (I can't merge that one as I made it).

Copy link
Member

@tbekolay tbekolay left a comment

Choose a reason for hiding this comment

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

LGTM, will reword the changelog item as per my comment.

CHANGES.rst Outdated

- Fixed bug #28 where search function hangs for many search terms.
Copy link
Member

Choose a reason for hiding this comment

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

The issue number is linked in the brackets afterwards, it shouldn't be mentioned in the changelog item as they should be self-contained. The links afterward are for people who want to dive deeper, they shouldn't ever have to go there.

Copy link
Member

Choose a reason for hiding this comment

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

Also, #28 is an issue so should be linked to #28 and both links should be in the same set of brackets separated by commas, not in two separate parentheses.

Fixed by applying the changes from sphinx-doc/sphinx#6394.
Specifically, the main div now has the `role="main" attribute.

Fixes #28.
@tbekolay tbekolay merged commit 107e90f into master Aug 5, 2019
@tbekolay tbekolay deleted the search-fix branch August 5, 2019 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Doc search broken for some queries
2 participants