Apply modal pattern to search box pop-up #1932
Conversation
| <div class="search-button__overlay"></div> | ||
| <div class="search-button__search-container">{% include "../components/search-field.html" %}</div> | ||
| </div> | ||
| <dialog class="search-button__search-container"> |
There was a problem hiding this comment.
Wondering if this class should be renamed
There was a problem hiding this comment.
I have a question in another comments if this should even be made an id, as the js assume there is only one I think. Don't trust me on Js.
| input.focus(); | ||
| input.select(); | ||
| input.scrollIntoView({ block: "center" }); | ||
| // if the input field is not the hidden one, then toggle its focus state |
There was a problem hiding this comment.
On the left side, the logic in this else-branch was always run. That was because the old way of opening the search pop-up did not automatically focus the search input field. However, when the search input field is inside a dialog, then when you open the dialog, it is automatically focussed. So if you then run the code below, you get a buggy sequence of actions:
- Call to
searchDialog.showModal()puts focus on search input field - Call to
input.blur()removes focus from search input field
So the solution is to put this toggle logic in an else-branch to handle the case when the search field is on already on the page (for example, the search results page, or a page with a persistent search bar).
| if (overlay) { | ||
| overlay.onclick = toggleSearchField; | ||
| } | ||
| // If user clicks outside the search modal dialog, then close it. |
There was a problem hiding this comment.
The default behavior for modal dialogs is to stay on the screen even if you click on the dialog's backdrop.
To me the old behavior seemed natural though - i.e., if the user clicks the backdrop, we remove the pop-up search bar. So I restore that behavior here.
| color: var(--pst-color-border); | ||
| } | ||
| } | ||
|
|
||
| .form-control { |
There was a problem hiding this comment.
I merged these rules in with the input selector above. As far as I can tell, I searched the code base, and the only place I could find this class being used is on the input element in the search-field.html component.
| } | ||
|
|
||
| // Shows off the keyboard shortcuts for the button | ||
| .search-button__kbd-shortcut { | ||
| display: flex; | ||
| position: absolute; |
There was a problem hiding this comment.
I think position: absolute was actually a bad idea because it allows the search terms in the input box to bleed into the span displaying the keyboard shortcut (think narrow screen rather than super long search string).
| padding-top: 0; | ||
| padding-bottom: 0; |
There was a problem hiding this comment.
These padding rules are not needed. The form already has 0 top and bottom padding.
src/pydata_sphinx_theme/theme/pydata_sphinx_theme/components/search-field.html
Outdated
Show resolved
Hide resolved
| @@ -6,7 +6,6 @@ | |||
| <input type="search" | |||
| class="form-control" | |||
| name="q" | |||
| id="search-input" | |||
There was a problem hiding this comment.
It's misleading to put an id on this element because this template can currently be rendered on the same page more than once. This definitely happens in the PST docs, for example, on the search results page.
|
this has been bugging me for so long, it looks much nicer thanks |
|
I started a discussion about how I would like to change the keyboard shortcut behavior. |
Coverage report
This PR does not seem to contain any modification to coverable code. |
This is very similar to #1932. Closes external issue Quansight-Labs/czi-scientific-python-mgmt#84 --------- Co-authored-by: M Bussonnier <bussonniermatthias@gmail.com>
Closes external issue Quansight-Labs/czi-scientific-python-mgmt#83.
Summary
This PR implements the pop-up search field as an HTML-native
<dialog>. This somewhat simplifies our implementation and brings accessibility affordances with it.I also introduced a couple visual changes, which fix #1714.
Before
After
Test plan
I tested this in Firefox, Chrome, and Safari on my Mac. I also tested it in Ubuntu + Firefox.
TODO:
There are at least six different presentations of the search input field that need to be tested:
As the persistent search is currently broken in production, it's also broken in this PR. How so? On the persistent search field example page, if you click the search button in the nav bar, or you press the keyboard shortcut (Ctrl + K), neither of those actions does anything because the "persistent" search field is hidden behind the mobile sidebar which only opens up if you click the hamburger icon.