-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 Block: Add icon only option with expandable search field #31719
Conversation
Size Change: +1.14 kB (0%) Total Size: 1.03 MB
ℹ️ View Unchanged
|
2895826
to
ee5e7e4
Compare
290157a
to
e118ef3
Compare
e16b2bc
to
d45182f
Compare
…y temporary until better alignment controls are in place.
…rent mode is selected.
I think this is now ready for review. With TwentyTwentyOne it is working correctly in the editor and front end. I've checked a handful of other themes and it also seems to be working okay. Note: some themes may need to tweak their styles to better support this visually, especially where border widths on the search field are used to match the button border widths. TODO:
|
This is testing well for me. There are some sizing issues with the collapsed button in blank canvas and twentytwenty theme: TwentyTwenty just looses the right border on the button. Not sure if this is something we can fix from the editor css level or not. In both cases it is just an issue in the editor, the front end displays as expected. |
This is excellent work, thanks so much for working on it. This is what I see: here's the frontend: I note that you've also seen the missing right border. That appears to be there because the input field isn't actually hidden, it's simply sized zero pixels wide, which for whatever reason doesn't compute to zero, but to 1px. I sense it's scaled as is to provide the nice animation when the field is untoggled. Sidenote: perhaps don't test in TwentyTwenty, though, it has increasingly outdated editor styles, and uses an incorrect method for loading editor styles which will break in the iframe. See also #18571. I'd recommend testing using a theme from https://github.com/WordPress/theme-experiments. Specifically, I would recommend optimizing for "Empty Theme" that's part of that repo. It provides zero editor styles, meaning you'll be able to build the baseline without interference from incorrect editor styles. Once you have it working, you can then look at theme style issues. In this case, I can't get the search field to appear on the frontend: Looking at the code, there appears to be a fair bit of frontend JS to make it work. That's not necessarily a problem, but it does appear to be doing this in a way that may be more complex than it needs to be. I'm not entirely sure how it's meant to work. In the past I've emulated this effect by having the input field be the only search interface, then collapse it to look like a button when unfocused, and look like a search field when focused. I imagine the new effort here is due to accessibility problems with that method? Alternatively now that we don't support IE11, could Nice work, excited to see this progress. Some additional notes on frontend/backend consistency which are technically unrelated to this PR, so you can ignore. But since you're here, I thought I'd mention it. This is the button in the editor: This is the frontend: The discrepancy happens for a few reasons that seem easy to fix. First off, here are the frontend styles: Here are the editor styles: Those are mostly the same, except the padding is being overridden by these: I think we can actually remove at least that editor-only padding, perhaps more. The search block used to be built using editor UI components ( The font size and button size are off because the Note the added font-size and line-height properties. That will make the editor and frontend look the same. Frontend: Editor: |
@jasmussen Thanks for the thorough feedback! I'll try testing with the emptytheme and fix up what you've found. I'm sure the frontend JS can be simplified significantly, I'll work on it. |
Thank you for working on this @apeatling, it's great! I share some of the concerns regarding the frontend script, it feels too complex. I experimented a bit with it and submitted a PR against this branch on #32387. That PR simplifies the frontend JS, removes some of the transitions logic from JS, and uses CSS for them. 👍 |
* Simplify frontend script * add transition to the editor * Apply width even for button only mode. Co-authored-by: apeatling <apeatling@gmail.com>
The JS has now been significantly simplified thanks to work from @aristath ❤️ . @jasmussen ready for another review when you get a chance. |
Very cool work. This is what I see: This didn't quite work how I expected it — I expected that simply focusing the search button would make the search field appear, because the search "button" actually was the search field. Kind of like this. I don't mind particularly the current behavior, and I can understand it might be necessary — so I would appreciate education on whether there might be some accessibility reason for why it should be this way. Is there a need for the ability to focus first the button, then invoke that with space, then transfer the focus to the input field? If there isn't, I still think that a click or simply focus on the "button", immediately expanding it to a search field, would be a better user experience. Otherwise, this one is really close, and will be a great win. Thank you Andy and Ari! |
Co-authored-by: Ari Stathopoulos <aristath@gmail.com>
I plan on getting back to this soon, I've had to prioritize some other work in the meantime. 👍 |
Not going to get back to this one any time soon, so closing out. |
If we could add search icon-only option when we add search on the navigation area, that would be great. Search icon takes because less space. This is very common people like to have search icon menu area. Thank you for your great work. |
Description
Fixes #31128
This PR reinstates the icon only option for the search block, and adds an option to display the search field when the button is activated.
How has this been tested?
Tested locally so far, fixtures have been updated.
Screenshots
In Editor:
On Front End
Types of changes
New Feature. This change adds a significant new feature to the search block.
Checklist:
*.native.js
files for terms that need renaming or removal).