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.

Sign up for GitHub

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

Jump to bottom

ENH: Search button shows up on mobile #832

Merged
merged 2 commits into from Jul 24, 2022
Merged

ENH: Search button shows up on mobile #832

merged 2 commits into from Jul 24, 2022

Conversation

choldgraf
Copy link
Collaborator

@choldgraf choldgraf commented Jul 22, 2022 •
edited

This PR makes our search button show up in header when on mobile. This means users can click to search with one click from any page on our documentation.

To accomplish this, I had to slightly re-work how the search implementation works. Here's a quick summary:

  • There are now two search buttons on the page, one just before the navbar_end section (which will only show up on wide screens), and one just before the navbar_center section (which will only show up on narrow screens)
  • The search overlay is now outside of our header and is now alongside our other overlays (for sidebars)
  • The CSS is re-worked accordingly
  • The JS now has a bit of extra logic to only find search buttons that are not display: none
  • I also cleaned up the JS a bit to use functions instead of executing immediately

Closes #824

@choldgraf choldgraf mentioned this pull request Jul 22, 2022
Merged
6 tasks
drammock
drammock approved these changes Jul 22, 2022
View reviewed changes
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.

changes make sense, and it works as expected on the PR site build. Nice! Just one question and one "remove crufty comments" request :)

src/pydata_sphinx_theme/theme/pydata_sphinx_theme/layout.html

<div class="search-button__wrapper">
<div class="search-button__overlay"></div>
<div class="search-button__search-container">
Copy link
Collaborator

Choose a reason for hiding this comment

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

previously this was {{ containerClass }}, I'm having trouble figuring out the reasons / implications of hard-coding it instead. Is it easy for you to explain?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the main reason I removed it is because it only seemed to be used in a single place in all of the theme, so I suspect in a previous iteration we had used it in multiple places, but then had removed one of those places. If we're only using it once, and if we were hard-coding it as a variable, I figured it'd be simpler to just remove the variable.

Copy link
Collaborator

Choose a reason for hiding this comment

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

makes sense, thanks!

src/pydata_sphinx_theme/theme/pydata_sphinx_theme/components/search-button.html Outdated
@@ -1,14 +1,5 @@
{# Search page has its own UI / UX for search #}
<!-- A button that will trigger a search field to be displayed on click. -->
{% set containerClass="search-button__search-container" %}

<!-- An overlay that will expand in the background and cause the search to disappear when clicked -->
Copy link
Collaborator

Choose a reason for hiding this comment

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

this comment, and the comment on line 1, both seem outdated / irrelevant now: the overlay was removed from this file, and the fact that the search page has its own UI for search matters, but doesn't matter here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

comments removed! good catch

@tupui
Copy link
Contributor

tupui commented Jul 22, 2022

Cool better experience for the user. Do I understand correctly that on mobile the design is fixed and not controlled anymore by what you put in navbar_end? Fine with me.
For the css I suppose we will see that in the other PR with the menu.

@choldgraf
Copy link
Collaborator Author

Do I understand correctly that on mobile the design is fixed and not controlled anymore

Yep, right now it's hard-coded to show up on mobile. I didn't think it was likely that somebody really wouldn't want a search button to show up, but if we get requests like this we can add in functionality to do so in the future. In general, I'd rather we start with fewer knobs to turn, and then add them iteratively only when we know people want them :-)

@choldgraf
Copy link
Collaborator Author

I'm going to merge this one in so that we can iterate in #834

@choldgraf choldgraf merged commit 2e1269f into pydata:main Jul 24, 2022
@jarrodmillman jarrodmillman added this to the 0.10 milestone Jul 26, 2022
@choldgraf choldgraf mentioned this pull request Aug 28, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drammock drammock drammock approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.10
Development

Successfully merging this pull request may close these issues.

Make our search button show in the header on mobile
4 participants
@choldgraf @tupui @drammock @jarrodmillman