Try possibly better method for Block Inserter Search focus #37793
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alexstine
added
[Type] Enhancement
|
Size Change: -3 B (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
|
@kevin940726 Not sure if this is how you envisioned it, but this seems messier. I do like how it doesn't rely on a selector anymore but I'm not sure this is good enough. I honestly think we'd be better off modifying the SearchComponent to auto focus true/false and just letting menu.js handle everything. I think what I changed in this PR is everything @youknowriad wanted to avoid in #37357. Let me know your thoughts. Thanks! |
kevin940726
reviewed
Jan 10, 2022
There was a problem hiding this comment.
Honestly, I think this is already a better solution than using class name selector. What specifically here should be avoided in your opinion?
Modifying the search component to auto focus is also a solution, just less robust, but might be a better solution in this context.
We could also remove the shouldFocusSearch prop and just re-export the imperative handle in InserterLibrary, but that might be overkill for now.
|
@kevin940726 I will make the changes tomorrow, Mac had to be wiped over the weekend. This is what I get for testing new features. 😬 Anyway, I kind of just thought the class method looked simpler but I guess I understand why this is better. The only thing I wish we could do is eliminate the prop and call the focusSearch function from the sidebar files. How would I go about exporting that again? It's defined in menu.js, does that mean I have to forward the prop all the way back to the sidebar files? |
Yes, I guess it's the simplest method. It's just two levels above though so it's probably not a big deal. For the record, I'm not completely against the selector method, just against the "class name" selector method, since that class names in my opinion are always implementation details specifically for styling and could change at any time. using a data-attribute selector is a good alternative since it'll then be an explicit API for this purpose. Using |
|
@kevin940726 Fair enough. I will see if I can pass it up a couple more levels. Coming from PHP/OOP, learning React in theory shouldn't be so hard but I am still learning the design principals. I agree, explicitness is always better than ease. At least the Inserter is more accessible now, I will try to make a more solid solution here. Although, one thought did come to mind. I am almost in favor of keeping the prop and code as is with it's current structure because all you have to do is import library and then pass the prop for focus. I know this is a stand-alone component or could be used as one, but I think it's kind of cool to be able to give users the choice if focus occurs or not in the search field. The other thing is if I added this to search component, it would give users the option to focus search which might actually be more useful down the line for us as well. Just bouncing some ideas around on how to best design this... |
youknowriad
reviewed
Jan 10, 2022
| return; | ||
| } | ||
| inserterRef.current.searchFocus(); | ||
| }, [ shouldFocusSearch ] ); |
There was a problem hiding this comment.
I think this effect is misplaced, it should remain where the existing effect is because the library is not necessarily a popover which means it doesn't need to move the focus in all usage.
Reading more, I guess you get around this by using a prop (shouldFocusSearch) but since we can have a ref with a focusSearch function outside, I'm not sure this is necessary.
|
@kevin940726 What are your thoughts now? Is this how you thought this should work? |
kevin940726
reviewed
Jan 11, 2022
packages/edit-post/src/components/secondary-sidebar/inserter-sidebar.js
Outdated
Show resolved
Hide resolved
alexstine
requested review from
adamziel,
draganescu,
noisysocks,
talldan and
tellthemachines
as code owners
…r-focus-method-for-block-inserter-search-focus
|
@kevin940726 Let's wait on the checks, but hopefully better now. |
kevin940726
approved these changes
Jan 11, 2022
kevin940726
deleted the
try/better-focus-method-for-block-inserter-search-focus
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Try using useImperativeHandle for Block Inserter Search focus.
How has this been tested?
Tested in Firefox with NVDA. Currently, changes only made to edit-post.
Screenshots
Types of changes
Code enhancement
Checklist:
*.native.jsfiles for terms that need renaming or removal).