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 block-level keyboard shortcuts #4436

Closed
wants to merge 20 commits into from
Closed

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Jan 12, 2018

Description

This PR aims to add all block-level shortcuts mentioned in #71.

  • Align center ^⌥C
  • Align left ^⌥L
  • Align right ^⌥R
  • Bullets ^⌥U
  • Ordered list ^⌥O
  • Heading 1-6 ^⌥1-6
  • Quote ^⌥Q

These are block level shortcuts, as opposed to Editable shortcuts, because they change block attributes other than the Editable value.

How Has This Been Tested?

  • Test alignment shortcuts on heading and paragraph. If already aligned, it should undo. Should work with multi-selection.
  • Test Heading shortcuts from paragraph to heading, and changing the level within a heading block. If already at the level, it should go back to a paragraph block. Also try multi-selection.
  • Test list shortcuts form paragraphs to list. Try changing the list type. If already the type, it should go back to paragraphs. Multi-selection should also work here. If two lists are selected, it will merge the lists.

Screenshots (jpeg or gifs if applicable):

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows has proper inline documentation.

@ellatrix ellatrix force-pushed the try/block-level-shortcuts branch from f851af9 to f25e33d Compare January 15, 2018 14:28
@ellatrix
Copy link
Member Author

Working a bit more on this...

  • Seems we need to keep multi-selection after transforms in general, so you can undo it smoothly.
  • Implementing all the necessary transforms and within-block transforms is challenging. It can't just be done in one setting. :/

So far the heading shortcut should work well, the rest is still in the works.

@ellatrix ellatrix added the [Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... label Jan 15, 2018
@ellatrix
Copy link
Member Author

The paragraph both has text align and block align toolbars. Which should the shortcuts app to?

@ellatrix
Copy link
Member Author

Ideally adding a component such as AlignmentToolbar should automatically add the shortcuts for the block, but I can't immediately find a way to do that, as we need access to the block node. @youknowriad @aduth I was wondering if any of you had any ideas for this problem, other than passing the block node all the way down. :)

@youknowriad
Copy link
Contributor

@iseulde Maybe you can use a Slot/Fill to add the shortcode in AlignmentToolbar but apply it at the block's level. Just an idea, not sure it works.

@ellatrix
Copy link
Member Author

@youknowriad Yeah, I was thinking about a portal, but ideally it would need to wrap the block. Will play around with it, thanks. :)

@ellatrix
Copy link
Member Author

I think we'll have to tie it to the document instead, because this is also a problem on the block list when the focus is in the inspector.

@ellatrix
Copy link
Member Author

Hm, that won't work either because the component won't be mounted when typing.

@ellatrix
Copy link
Member Author

I mean, neither attaching to the document, nor the portal will work, because the alignment toolbar won't be mounted at all. It will have to be added as settings I think...

@ellatrix ellatrix force-pushed the try/block-level-shortcuts branch 3 times, most recently from 448e1fa to fc83895 Compare January 16, 2018 20:08
@ellatrix
Copy link
Member Author

This is ready for an initial review (for my own sanity).

@ellatrix
Copy link
Member Author

ellatrix commented Jan 16, 2018

Some notes:

  • Multi-selection should ideally persist. I'll look into this next.
  • There should be an easy way to include alignment shortcuts.
  • A snapshot test is failing, but I'm unsure how to correct.
  • Indented lists are failing, I'll rebase after Fix list to paragraph transform #4460.

@ellatrix ellatrix requested review from youknowriad and mcsf January 16, 2018 20:15
@@ -248,6 +249,21 @@ registerBlockType( 'core/paragraph', {
),
},
],
to: [ 'left', 'center', 'right' ].map( ( value ) => ( {
type: 'shortcut',
shortcut: value.charAt( 0 ),
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth adding an inline comment; this is giving me clever code vibes.

Aside: Could this be value[ 0 ]?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe I'll do map( { left: 'l' } ) etc. instead then... :) What do you mean with value[ 0 ]? It's not an array?

Copy link
Contributor

Choose a reason for hiding this comment

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

Per MDN (string, §Character access), array-like access was introduced in ES5, so I assume it safe to use. Both forms should be equivalent.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had no idea this was possible. :)

shortcut: value.charAt( 0 ),
transform( attributes ) {
const firstAlign = first( attributes ).align;
const isSame = attributes.every( ( { align } ) => align === firstAlign );
Copy link
Member

Choose a reason for hiding this comment

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

My unitiated perspective: What is the shape of attributes ? I would have expected an object for a single block, but this looks to be an array. Are these an array for blocks in a multi-selection? A bit unfortunate overloading of the term attributes between an object of a single block, and an array of multiple blocks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's similar in other transforms. Elsewhere it has been named blockAttributes, which doesn't really feel any better... Any ideas?

Copy link
Contributor

@mcsf mcsf Jan 17, 2018

Choose a reason for hiding this comment

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

Ah, I rambled on about the same thing in a different hunk. :) See #4436 (comment)

@@ -179,6 +182,7 @@ class Tooltip extends Component {
aria-hidden="true"
>
{ text }
{ shortcut && <span className="components-tooltip__shortcut">{ getAccessCombination( shortcut ) }</span> }
Copy link
Member

Choose a reason for hiding this comment

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

Should the concept of a keyboard shortcut be baked into a general tooltip UI component?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think so, as it has distinct styles. See also #4411.

Copy link
Member

Choose a reason for hiding this comment

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

I think so, as it has distinct styles. See also #4411.

The question was intended as less "Do we need tooltips in the editor to have shortcut-specific behavior?" and more in the abstract "When I use a Tooltip on some page in the admin, do I have the expectation that it supports keyboard shortcut functionality?"

(With the expectation that all of components is meant to be purely generic)

In these cases I might like if we could create a wrapper component, similar to the separation between IconButton and Button.

Copy link
Member Author

Choose a reason for hiding this comment

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

Would it make more sense if the getAccessCombination piece is part of IconButton, which would otherwise this new wrapper component?

Copy link
Member

Choose a reason for hiding this comment

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

I don't see any inherent relationship between an icon button and keyboard combinations, so no, it doesn't really make sense to me.

*/
const { userAgent } = window.navigator;

const isMac = userAgent.indexOf( 'Mac' ) !== -1;
Copy link
Member

Choose a reason for hiding this comment

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

We use a different test elsewhere, and should seek to consolidate or create a shared utility:

const isMac = window.navigator.platform.toUpperCase().indexOf( 'MAC' ) >= 0;

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, did not realise.

Copy link
Member Author

Choose a reason for hiding this comment

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

Commented on #3755 (comment). I wouldn't like us to have a lot of different keyboard combinations that are not rememberable.

/**
* Browser dependencies
*/
const { userAgent } = window.navigator;
Copy link
Member

Choose a reason for hiding this comment

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

Is the destructuring worthwhile if we're using it in one spot?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a bit strange indeed if the use immediately follows, but I find it clear to state browser dependencies.

@@ -62,15 +70,45 @@ class BlockList extends Component {
document.addEventListener( 'copy', this.onCopy );
document.addEventListener( 'cut', this.onCut );
window.addEventListener( 'mousemove', this.setLastClientY );
// Needs document to also work in inspector.
// In other words, if there is selection, but no focus.
document.addEventListener( 'keydown', this.onKeyDown );
Copy link
Member

Choose a reason for hiding this comment

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

Is it viable to use the KeyboardShortcuts component instead?

https://github.com/WordPress/gutenberg/tree/master/components/keyboard-shortcuts

Might also help with the platform normalization:

Mousetrap 1.4 introduced a generic mod helper which lets you set cross platform shortcuts.

Mousetrap.bind('mod+s', _saveDraft);

On Mac this ends up mapping to command+s whereas on Windows and Linux it maps to ctrl+s.

https://craig.is/killing/mice

Copy link
Member Author

Choose a reason for hiding this comment

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

How do we dynamically change those? Rebind as the selection changes? There's also no concept of access in Mousetrap, unlike TinyMCE.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, we're also not using this component anywhere else, so I'm wondering why now.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, we're also not using this component anywhere else, so I'm wondering why now.

We are:

<KeyboardShortcuts
bindGlobal
eventName="keyup"
shortcuts={ {
'alt+f10': this.focusToolbar,
} }
/>

<KeyboardShortcuts shortcuts={ {
[ shortcuts.toggleEditorMode.value ]: this.toggleMode,
} } />


// Check if we received blocks or attributes.
if ( result.uid || Array.isArray( result ) ) {
onReplace( this.blocks.map( ( { uid } ) => uid ), castArray( result ) );
Copy link
Member

Choose a reason for hiding this comment

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

Minor: Lodash's _.map property shorthand can be nice for this sort of thing:

map( this.blocks, 'uid' )

return;
}

if ( ! isAccess( event ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Any reason not to merge into previous condition?

Copy link
Member Author

Choose a reason for hiding this comment

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

No :)

}

componentWillReceiveProps( nextProps ) {
const prevCommonName = this.commonName;
Copy link
Member

Choose a reason for hiding this comment

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

Could we consider moving the logic for handling the shortcuts to a different component? This doesn't seem particularly relevant to the block listing, and would be easy enough to transport into a separate (non-visual) component which is rendered as a child of the BlockList root return value.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will, if the general direction of this PR seems good.

export function getPossibleShortcutTransformations( name ) {
const transformsFrom = getBlockTypes()
.reduce( ( acc, blockType ) => [ ...acc, ...get( blockType, 'transforms.from', [] ) ], [] )
.filter( isTransformForBlockSource( name, 'shortcut', false ) );
Copy link
Contributor

Choose a reason for hiding this comment

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

That reduction is actually of a specific kind: it's a mapping.

-  .reduce( ( acc, blockType ) => [ ...acc, ...get( blockType, 'transforms.from', [] ) ], [] )
+  .map( ( blockType ) => get( blockType, 'transforms.into' ) )

My second point was more relevant before realizing the reduce can just become a cheaper map, but: instead of filtering the reduction for things that are applicable transforms, can we instead first filter the block types for things that have applicable transforms, and then construct the transforms set?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, those two are not equal. Is that what you mean in the second point then? I'm find either way. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, yes. It's flatMap, actually.

My second point was above whether there's an efficiency gain in filtering first.

...'23456'.split( '' ).map( ( level ) => ( {
type: 'shortcut',
shortcut: level,
transform( attributes ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The term attributes is misleading, as everywhere else in Gutenberg it means "the set of attributes for a single block or block type", whereas here it's "the sets of attributes", plural, one for each block.

Copy link
Contributor

Choose a reason for hiding this comment

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

From an API design standpoint, we're reaching a tough spot. On one hand, it's easy to see why we'd want a set of sets of attributes, since we'd like to support keyboard shortcuts that affect multiple blocks. On the other hand, merely from looking at a block type's transforms, there is nothing intrinsically setting apart 'shortcut' from other transform types in such a way that it's obvious that it should receive a set of sets of attributes while the other transform types receive a single set of attributes.

A simple solution could be to pass blocks rather than map( blocks, 'attributes' ) to the transform, since that would make the transform more legible:

transform( blocks ) {
  const firstNodeName = first( blocks ).attributes.nodeName; // and so on

Side thought: if we decided that we wanted a simpler and less permissive API that only allowed a 1:1 mapping from selected block to transformed block, then the transform here could be rewritten to operate on a single block. In that case, passing attributes would be consistent in that it's a single set of attributes. The transform would look like:

transform( attributes ) {
  const foo = computeFrom( attributes );
  return createBlock( foo.name, foo.attributes );
}

The transform caller (as of now, BlockList) would be the one calling the transform for each selected block:

-  const result = transform.transform( map( this.blocks, 'attributes' ) );
+  const result = this.blocks.map( transform.transform );

However, the current, more feature-complete implementation requires more than that, so the point is moot.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. I was following what had been done before for multi selection transform on e.g. lists, where an array of attributes was passed as opposed to an array of blocks. See #3357 (cc @jorgefilipecosta @youknowriad). I'm fine with passing blocks instead of an array of attribute sets.

...[ 'OL', 'UL' ].map( ( tag ) => ( {
type: 'shortcut',
blocks: [ 'core/paragraph' ],
shortcut: tag.charAt( 0 ).toLowerCase(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless we centralize shortcut declarations in one place, perhaps add a line comment spelling out which shortcuts this adds, to assist the reader and to improve codebase searchability: // Keyboard shortcut: access+o, access+u

Copy link
Member Author

Choose a reason for hiding this comment

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

I will do the same as the alignment here, for readability.

// Merge list.
return createBlock( 'core/list', {
nodeName: tag,
values: attributes.reduce( ( acc, { values } ) => [ ...acc, ...values ], [] ),
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a good place for lodash#flatMap, per #3185, even though this isn't a hot path like that of the mentioned PR.

@@ -248,6 +249,21 @@ registerBlockType( 'core/paragraph', {
),
},
],
to: [ 'left', 'center', 'right' ].map( ( value ) => ( {
type: 'shortcut',
shortcut: value.charAt( 0 ),
Copy link
Contributor

Choose a reason for hiding this comment

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

Per MDN (string, §Character access), array-like access was introduced in ES5, so I assume it safe to use. Both forms should be equivalent.

shortcut: value.charAt( 0 ),
transform( attributes ) {
const firstAlign = first( attributes ).align;
const isSame = attributes.every( ( { align } ) => align === firstAlign );
Copy link
Contributor

@mcsf mcsf Jan 17, 2018

Choose a reason for hiding this comment

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

Ah, I rambled on about the same thing in a different hunk. :) See #4436 (comment)

this.commonName = firstName;
} else {
delete this.commonName;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What if we want to right-align a selection of paragraphs and images?

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, this is getting complicated. :) Transforms on different block types have not been done before, but it doesn't sound like a bad idea in some cases. In the case of paragraphs and images though, it means something different (align vs text-align).

},
onChange( uid, attributes ) {
dispatch( updateBlockAttributes( uid, attributes ) );
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Tangent to the PR, but I really dislike ( dispatch ) => ( { onBlah( x, y, z ) { dispatch( doBlah( x, y, z ) ); } } ) when we have { onBlah: doBlah }.

export function isAccess( event, character ) {
if ( isMac ) {
if ( ! event.ctrlKey || ! event.altKey ) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Though it makes no real difference, strictly speaking this should be return false.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, spotted that earlier on too :)

}

return character ? event.keyCode === character.toUpperCase().charCodeAt( 0 ) : true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Two optional refactors:

const modifierKey = isMac ? 'ctrlKey' : 'shiftKey';
if ( ! event[ modifierKey ] || ! event.altKey ) {
  return false;
}
return ! character || ( event.keyCode === character.toUpperCase().charCodeAt( 0 ) );

return false;
}

return character ? event.keyCode === character.toUpperCase().charCodeAt( 0 ) : true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: can extract function toKeyCode( char ) { return char.toUpperCase().charCodeAt( 0 ); }

Copy link
Member Author

Choose a reason for hiding this comment

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

Readability? Or for what reason?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, merely that. My personal preference, anyway. The function name acts as documentation, essentially.

@mcsf
Copy link
Contributor

mcsf commented Jan 17, 2018

Right now I'm able to make a heading out of the contents of a li:

  1. Add a list block
  2. Add two items
  3. With the caret at the end of the second item, press access+2

Resulting markup:

<ul>
    <li>Hello</li>
    <li>
        <h2>Bye</h2>
    </li>
</ul>

screen shot 2018-01-17 at 14 35 13

@ellatrix
Copy link
Member Author

@mcsf Ah, that's the case in master too. The shortcut is present in TinyMCE too, so it should be disabled.

@ellatrix ellatrix force-pushed the try/block-level-shortcuts branch from 1b01ec5 to f61c0a3 Compare January 17, 2018 20:59
@ellatrix
Copy link
Member Author

I think this is ready for another look. :)

Copy link
Contributor

@mcsf mcsf left a comment

Choose a reason for hiding this comment

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

Since the last time, I'm running into some bugs:

  • If I add a paragraph, then do access+2, it converts it to a heading with no issues. From there, I can keep changing levels with access+3, etc., or revert to paragraph.

  • However, if I add a Heading (via Inserter or ##), then press access+3, the level is changed but the text is lost.

  • Same with lists: start one with - , add one or more items, then try the shortcuts to convert between ordered/unordered or back to paragraph. Content is lost in all these cases. Content is not lost if using multi-selection of paragraphs, or if I have partial text selection of a list's items.

screen shot 2018-01-18 at 08 14 19

@@ -8,16 +8,19 @@ const ALIGNMENT_CONTROLS = [
{
icon: 'editor-alignleft',
title: __( 'Align left' ),
shortcut: 'l',
Copy link
Contributor

Choose a reason for hiding this comment

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

What if blocks/alignment-shortcuts exported the following constant:

export const shortcuts = { left: 'l',};
export default map( shortcuts, ( shortcut, value ) => 

… so that, here, we could just reuse shortcuts.left, etc., instead of duplicating constants?

(Same goes for blocks/block-alignment-toolbar.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice idea!

@@ -192,6 +192,9 @@ export default class Editable extends Component {
onInit() {
this.updateFocus();
this.registerCustomFormatters();

// Remove all default block-level shortcuts.
'123456789'.split( '' ).forEach( ( number ) => this.editor.shortcuts.remove( `access+${ number }` ) );
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know TinyMCE enough, but are we to assume from this hunk that there isn't a way to avoid the shortcuts being set up in the first place?

Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -79,6 +85,19 @@ registerBlockType( 'core/heading', {
} );
},
},
...'23456'.split( '' ).map( ( level ) => ( {
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious, why is h1 excluded? It's still offered as an option in the inspector.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, H1 should be equivalent to the title. I have no idea why it is an option in the inspector.

Copy link
Member Author

Choose a reason for hiding this comment

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

So in the future, if the title is a block, access+1 should convert to a title block, if there isn't one already.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds great.


return acc;
}, [] );
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Personal taste, but IMO this is a great place for a flatMap too:

return flatMap( blocks, ( { attributes: { value, citation } } ) => [
  ...value.forEach( ( paragraph ) => createBlock(  ) ),
  ...( citation ? [ createBlock(  ) ] : [] ),
] );

<BlockListShortcuts
selectedBlock={ selectedBlock }
multiSelectedBlocks={ multiSelectedBlocks }
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Something to look out for in #3745, @aduth, in order to avoid nested shortcut declarations.


const { getAccessCombination } = keycodes;

const combination = getAccessCombination( 'm' );
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't scale if more shortcuts are added to the module. I don't really mind for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not immediately sure what solution there is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Off the top of my mind:

const shortcuts = {
  toggleEditorMode: 'access+m',
};

const modifiers = {
  access: getAccessCombination,
}

const computed = reduce( shortcuts, ( acc, shortcut, shortcutName ) => {
  const keys = shortcut.split( '+' );
  // assumes only one modifier:
  const [ modifier, ...chars ] = keys;
  const combination = modifiers[ modifier ]( ...chars ); // obvs should check existence first
  acc[ shortcutName ] = {
    value: combination.toLowerCase(),
    label: combination,
  };
  return acc;

  // but could be:
  // const [ modifiers, chars ] = partition( keys, isModifier );
  // and somewhere:
  // const combination = combine( eligibleModifiers )( ...chars )
} );

export default computed;

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not entirely sure now what you mean with scaling. We should only have two possible combinations in the future: access+CHARACTER and meta+CHARACTER, the former could be used across Gutenberg and plugins, the latter is only the overwrite default behaviour or implement well-known combinations (limited amount).

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant if more shortcuts are added to that file.

Only having access and meta as modifiers is good. I wondered if any other combo with shift could creep in, hence the convoluted suggestion at the end of my code snippet.

@ellatrix
Copy link
Member Author

@mcsf Haha, that's because the Editable fields are not continuously synced. :/ It seems we first need to remove the focus, which is unfortunate.

@ellatrix
Copy link
Member Author

Is the error related? I can't seem to reproduce.

@ellatrix
Copy link
Member Author

@mcsf I'm also not entirely sure how to fix that, other than waiting for a fix for the syncing, or dirty fix by forcing a sync on isAccess( event ) in Editable.

@@ -52,7 +53,10 @@ function getEmbedBlockSettings( { title, icon, category = 'embed', transforms, k
},
},

transforms,
transforms: {
from: transforms.from,
Copy link
Contributor

@mcsf mcsf Jan 18, 2018

Choose a reason for hiding this comment

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

What if someone passes transforms with a to? Shouldn't we use ...transforms, to: { ...transforms.to, bla }?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is not extensible outside of this file, so if more are added, the code can be adjusted? Either way, I'm fine with changing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not extensible outside of this file

Yeah, disregard then. :)

@mcsf
Copy link
Contributor

mcsf commented Jan 18, 2018

Is the error related? I can't seem to reproduce.

The startContainer error happens when I backspace out of a heading or quote into the previous block, but it's also present in master. The isCollapsed one I'm not able to repro.

@mcsf I'm also not entirely sure how to fix that, other than waiting for a fix for the syncing, or dirty fix by forcing a sync on isAccess( event ) in Editable.

The sync forcing does solve the problem for me. I'm not totally confident assessing whether that fix could be problematic, but we do have precedents in Editable, so I'm open to it.

@ellatrix ellatrix force-pushed the try/block-level-shortcuts branch from 6bc21e8 to 99c70d7 Compare February 2, 2018 12:48
@ellatrix
Copy link
Member Author

ellatrix commented Feb 2, 2018

Rebased

@aduth
Copy link
Member

aduth commented Feb 8, 2018

If I understand correctly, by implementing these as transforms, are we replacing a block with a new variation when in many cases these are merely attributes changes?

(Should transforms accommodate this usage?)

@@ -529,6 +532,10 @@ export default class RichText extends Component {
const dom = this.editor.dom;
const rootNode = this.editor.getBody();

if ( isAccess( event ) ) {
this.fireChange();
Copy link
Member

Choose a reason for hiding this comment

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

Haven't followed the logic, but in what way is an access keydown a change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Otherwise the state would not be up to date at the time of the command. This Is an ugly fix. There are PRs in the works that sync RichText state continuously though, and then this would no longer be needed.

@aduth
Copy link
Member

aduth commented Sep 13, 2018

Is it likely this will be resumed, or can it be closed?

@aduth aduth added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Sep 13, 2018
@aduth
Copy link
Member

aduth commented Oct 15, 2018

Closing as stale without response. It can be reopened if development is expected to continue, or a new pull request can be submitted.

@aduth aduth closed this Oct 15, 2018
@aduth aduth deleted the try/block-level-shortcuts branch October 15, 2018 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants