-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Navigate regions: use React events for shortcuts (portal bubbles & contextual) #33633
Conversation
Size Change: -82 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
|
||
useEffect( () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be using useRefEffect
here because it will be called when the ref changes.
I actually think region navigation shortcuts should be global and just catch all events on |
onKeyDown( event ) { | ||
if ( | ||
shortcuts.previous.some( ( { modifier, character } ) => { | ||
return isKeyboardEvent[ modifier ]( event, character ); | ||
} ) | ||
) { | ||
focusRegion( -1 ); | ||
} else if ( | ||
shortcuts.next.some( ( { modifier, character } ) => { | ||
return isKeyboardEvent[ modifier ]( event, character ); | ||
} ) | ||
) { | ||
focusRegion( 1 ); | ||
} | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if this becomes the way to write shortcuts, we'll probably need better APIs to make this more friendly. As a shortcut developer, I know three things: the shortcut combination, the element on which to apply it and the callback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll think a bit more about how we could simplify connecting this with React events.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we'll most likely need a better API. Currently we have an API to change existing shortcut combinations, but not to add them to a certain region. Based on the section (global/editor, block, text etc.) we can add it to the right element.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After a discussion with Ella, I'm fine with these refactorings in order to simplify the framing of the editor. I think we should be careful with breaking changes though.
Description
Similar to #32323.
Moves shortcut handlers from native events to React events for two reasons:
document
). This is important because you may have other things on the page beside the block editor, or multiple block editors. Shortcuts should only work if focus is within a certain context. Using React evens works nicely because they bubble both through the iframe and other portals such as the inspector.How has this been tested?
Screenshots
Types of changes
Checklist:
*.native.js
files for terms that need renaming or removal).