Skip to content
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

Add keyboard options to blocks #3400

Closed
wants to merge 6 commits into from
Closed

Conversation

spacedmonkey
Copy link
Contributor

Add options to blocks to move and delete blocks.

Fixes #3280 #3325

@googlebot googlebot added the cla: yes Signed the Google CLA label Oct 1, 2019
@spacedmonkey
Copy link
Contributor Author

This PR has the following issues.

  • Doesn't work on AMP text blocks
  • Block with text editing and selected text, currently move.
  • Better handling for when arrow keys are held down.

@spacedmonkey
Copy link
Contributor Author

This PR is far from complete. But I have added for review as I would like some thoughts on this.

The biggest issue, is not moving a block left and right when editing text. I have a very dirty work around for this, but I want something a little more full proof way of knowing a user is editing. This is hard to do, when it is a mixture of core and amp story blocks. I was thinking of having a isEditing field in the redux store. But this would require hook into core blocks and changing all amp story blocks. Also feel like this will not be full proof.

return;
}

if ( classList.contains( 'editor-rich-text__editable' ) && classList.contains( 'is-selected' ) ) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't there a isSelected prop?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that I can see

attributes: {ordered: false, values: "", addedAttributes: 0, opacity: 100, positionTop: 30, …}
clientId: "b1540913-690e-4e23-8a0a-8f531c77301e"
innerBlocks: []
isValid: true
name: "core/list"

Copy link
Collaborator

left = -1;
break;
case DELETE:
removeBlock( selectedBlock.clientId );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the existing keyboard shortcut for removing blocks?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #3280

@swissspidy
Copy link
Collaborator

The biggest issue, is not moving a block left and right when editing text.

When I'm typing in a text block, I wouldn't expect arrow keys to move the block 🤔

@spacedmonkey
Copy link
Contributor Author

spacedmonkey commented Oct 2, 2019

When I'm typing in a text block, I wouldn't expect arrow keys to move the block

Yes, that is what I am saying. When in text block is selected and the cursor is blinking in an editable field, left and right arrows, should move the cursor and not the block.

@miina
Copy link
Contributor

miina commented Oct 2, 2019

I was thinking of having a isEditing field in the redux store. But this would require hook into core blocks and changing all amp story blocks.

Hmm, maybe that's something we might possibly want to do anyway -- to implement edit vs drag-mode for all the text blocks (which would require extending all the core blocks anyway)? Currently, only the CTA block and Text block have the feature of being able to drag from anywhere while not editing and being able to edit after two clicks (and losing the dragging ability).

Wondering if the core blocks allowing text (quote, list, etc.) might require the same.

Also, I wonder if instead of isEditing field in the Redux store using React Context might be an option here. Not sure how much additional code re-rewriting this might require though.

Thoughts?

@swissspidy
Copy link
Collaborator

That reminds me of #2788 and WordPress/gutenberg#17088.

@barklund
Copy link
Contributor

barklund commented Oct 2, 2019

I just noticed that the KeyboardShortcuts component of @wordpress/components already handles text input focus and makes sure not to invoke the events if focus is inside a text field. You might be able to use it too (as I'm trying to use it for the context menu).

@spacedmonkey
Copy link
Contributor Author

A good example of Keyboard shortcuts usable for something similar in the gutenberg can be found here.

@spacedmonkey
Copy link
Contributor Author

I have updated the PR to use the KeyboardShortcuts component. I am still having the same issue editting text based blocks. The workaround I put in place doesn't work for many of the core blocks, as the classnames do not match. We are going to have to think of a new system of selecting blocks, maybe with a double click..

} );

/**
* Higher-order component that adds right click handler to each inner block.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Higher-order component that adds right click handler to each inner block.
* Higher-order component that adds a keyboard navigation handler to each inner block.

*/
export default createHigherOrderComponent(
( BlockEdit ) => {
return applyWithDispatch( ( props ) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could use hooks here


let top = 0;
let left = 0;
switch ( keyCode ) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs some testing, but it would be nice if one could move in bigger steps when holding down an additional key, e.g. ctrl, alt, or cmd. If I am not mistaken this kind of behavior is common in other applications when moving elements with the keyboard

@schlessera schlessera deleted the feature/move-block branch March 7, 2020 06:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Signed the Google CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unexpected behavior when pressing the delete / backspace key in AMP Story editor
5 participants