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

Handle varied search field configurations #807

Merged
merged 9 commits into from
Jul 13, 2022

Conversation

drammock
Copy link
Collaborator

This makes the search shortcut functionality a bit nicer for sites that choose to use a persistent / always-visible search field.

it pulls out some of the work from #777, which suffered from scope creep.

@drammock
Copy link
Collaborator Author

the CI failures here are the prerelease problems with mistune, already reported in #805. Just realized that I forgot to add a special page to the demo docs for testing the persistent search field; will do that now.

@drammock
Copy link
Collaborator Author

drammock commented Jul 13, 2022

OK, I've added a test page to the demo docs... only to realize that it provides an opportuntity for interactive dev testing that was already available on the search results page. So IMO it would be fine to revert 2a925b0 and e11ddb6 but I can also see a case for keeping it (i.e., having a test page specifically titled that way might remind/encourage us to actually check it periodically)

Co-authored-by: Chris Holdgraf <choldgraf@gmail.com>
@choldgraf
Copy link
Collaborator

This is looking really good to me! I think it is pretty much ready to go

One quick thought - this might be an edge case, but if the user is on mobile and they are on a page with a persistent search bar in the sidebar, and they click the search button, then nothing will happen because the search field will be activated but the sidebar will be hidden already. Maybe that is uncommon-enough that it is not worth special casing though.

@drammock
Copy link
Collaborator Author

That's a very good point. Hopefully it is a rare case and most sites will use the button. But if they don't, this actually feels pretty broken. If like to at least add a warning to the docs about this

This was referenced Jul 13, 2022
@drammock
Copy link
Collaborator Author

OK, here's the rendered docs section with the warning: https://pydata-sphinx-theme--807.org.readthedocs.build/en/807/user_guide/configuring.html#search-bar-search-button

Feel free to push or suggest/commit wording changes if you like

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.

This looks good to me! We've already had a bit of iteration on this in #777 so let's merge this in and give people a chance to try it out!

@choldgraf choldgraf changed the title handle varied search field configurations Handle varied search field configurations Jul 13, 2022
@choldgraf choldgraf merged commit 6dab1f9 into pydata:main Jul 13, 2022
@drammock drammock deleted the fix-search-shortcuts branch July 13, 2022 19:27
@jarrodmillman jarrodmillman added this to the 0.10 milestone Jul 26, 2022
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.

3 participants