-
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
Block arrow key navigation #595
Conversation
See #520. |
These are issues where I think it would make much more sense to have a Anyway, this seems a fine temporary solution. |
Hm, there's also an issue with the link boundary stuff adding a zero width space so that the caret is not at the start or end. |
blocks/components/editable/index.js
Outdated
}, null ); | ||
|
||
if ( targetNode ) { | ||
targetNode.focus(); |
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 would prefer to avoid the imperative approach here and rely on the redux flow for focusing the target. something like:
const blockUid = targetNode.closest( '[data-block-uid]' ).attr( 'data-block-uid' );
const editable = targetNode.closest( '[data-block-editable]' ).attr( 'data-block-editable' );
const offset = before ? -1 : 0;
onFocus( blockUid, { editable, offset } );
This assumes we add the data-block-uid
to each block container and a data-block-editable
(optional) to blocks with multiple editables.
(I'm secretly hoping we could use these attributes to drop the focus
, updateFocus
and mergeWithPrevious
completely from the block edit
function.)
@aduth Thoughts
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 like the idea of dispatching to Redux to shift focus, and the idea to eliminate focus props from the edit
implementation. I'm not so sure of assuming attributes on an ancestor node and using them as action properties. But I've not digested this deeply and don't have alternatives to suggest at the moment.
With #520 closed, perhaps we should close this also? |
WIP. Does not set caret at start or end based on the direction yet.