Skip to content

Inserter: Fix the insertion point #994

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.

Sign up for GitHub

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

Merged
youknowriad merged 5 commits into from
Jun 2, 2017
Merged

Inserter: Fix the insertion point #994

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ecbee44
Inserter: Fix the insertion point styling
youknowriad Jun 2, 2017
1df84c7
Inserter: Avoid clearing the selection if we click outside the editor
youknowriad Jun 2, 2017
2186d41
Inserter: Allow inserting blocks after multi-selection
youknowriad Jun 2, 2017
a2e90db
Blocks: Clear selection when clicking on the canvas
youknowriad Jun 2, 2017
3d16e47
Blocks: drop stopPropagation when clicking on blocks
youknowriad Jun 2, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions editor/inserter/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { IconButton } from 'components';
* Internal dependencies
*/
import InserterMenu from './menu';
import { getSelectedBlock } from '../selectors';
import { getBlockSelectionEnd, getSelectedBlock } from '../selectors';
import { insertBlock, clearInsertionPoint } from '../actions';

class Inserter extends wp.element.Component {
Expand Down Expand Up @@ -41,10 +41,17 @@ class Inserter extends wp.element.Component {

insertBlock( slug ) {
if ( slug ) {
const { selectedBlock, onInsertBlock } = this.props;
const { selectedBlock, lastMultiSelectedBlock, onInsertBlock } = this.props;
let insertionPoint = null;
if ( lastMultiSelectedBlock ) {
insertionPoint = lastMultiSelectedBlock;
} else if ( selectedBlock ) {
insertionPoint = selectedBlock.uid;
}

onInsertBlock(
slug,
selectedBlock ? selectedBlock.uid : null
insertionPoint
);
}

Expand Down Expand Up @@ -90,6 +97,7 @@ export default connect(
( state ) => {
return {
selectedBlock: getSelectedBlock( state ),
lastMultiSelectedBlock: getBlockSelectionEnd( state ),
};
},
( dispatch ) => ( {
Expand Down
14 changes: 10 additions & 4 deletions editor/inserter/menu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import { TAB, ESCAPE, LEFT, UP, RIGHT, DOWN } from 'utils/keycodes';
* Internal dependencies
*/
import './style.scss';
import { getSelectedBlock } from '../selectors';
import { getBlockSelectionEnd, getSelectedBlock } from '../selectors';
import { setInsertionPoint, clearInsertionPoint } from '../actions';

class InserterMenu extends wp.element.Component {
Expand Down Expand Up @@ -68,10 +68,15 @@ class InserterMenu extends wp.element.Component {
}

hoverBlock() {
const { lastMultiSelectedBlock, selectedBlock } = this.props;
let insertionPoint = null;
Copy link
Member

@aduth aduth Jun 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

In a subsequent pull request, we should think about consolidating or simplifying some of this shared logic between the menu and the root component. I still think it's a bit bizarre how we assign insertion point in the hover intent, but IIRC the reasoning was to show the indicator in the post.

Copy link
Contributor Author

@youknowriad youknowriad Jun 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, right! Actually, I thought about passing a prop from the inserter to themenu to void recomputing.

if ( lastMultiSelectedBlock ) {
insertionPoint = lastMultiSelectedBlock;
} else if ( selectedBlock ) {
insertionPoint = selectedBlock.uid;
}
return () => {
this.props.setInsertionPoint(
this.props.selectedBlock ? this.props.selectedBlock.uid : null
);
this.props.setInsertionPoint( insertionPoint );
};
}

Expand Down Expand Up @@ -306,6 +311,7 @@ export default connect(
( state ) => {
return {
selectedBlock: getSelectedBlock( state ),
lastMultiSelectedBlock: getBlockSelectionEnd( state ),
};
},
{ setInsertionPoint, clearInsertionPoint }
Expand Down
8 changes: 1 addition & 7 deletions editor/modes/visual-editor/block-list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
* External dependencies
*/
import { connect } from 'react-redux';
import clickOutside from 'react-click-outside';

/**
* Internal dependencies
Expand Down Expand Up @@ -56,10 +55,6 @@ class VisualEditorBlockList extends wp.element.Component {
this.setState( { selectionAtStart: null } );
}

handleClickOutside() {
this.props.clearSelectedBlock();
}

render() {
const { blocks, insertionPoint } = this.props;
const insertionPointIndex = blocks.indexOf( insertionPoint );
Expand Down Expand Up @@ -106,9 +101,8 @@ export default connect(
selectionEnd: getBlockSelectionEnd( state ),
} ),
( dispatch ) => ( {
clearSelectedBlock: () => dispatch( { type: 'CLEAR_SELECTED_BLOCK' } ),
onMultiSelect( { start, end } ) {
dispatch( { type: 'MULTI_SELECT', start, end } );
},
} )
)( clickOutside( VisualEditorBlockList ) );
Copy link
Member

@mtias mtias Jun 2, 2017

Choose a reason for hiding this comment

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

This means we can't deselect a block by clicking on the canvas now.

Copy link
Contributor Author

@youknowriad youknowriad Jun 2, 2017

Choose a reason for hiding this comment

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

Yes, and I think you explicitly suggested this somewhere? Am I wrong?

Copy link
Member

@mtias mtias Jun 2, 2017

Choose a reason for hiding this comment

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

What I meant is that focused and selected should be two different states, where focus also means selected. Clicking on the canvas should deselect the block, what I meant before is that clicking on the inserter should unfocus but not deselect (same for sidebar inspector).

Copy link
Contributor Author

@youknowriad youknowriad Jun 2, 2017

Choose a reason for hiding this comment

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

Ok, so we shouldn't leverage onClickOutside for this but maybe onClick on the canvas

Copy link
Contributor Author

@youknowriad youknowriad Jun 2, 2017

Choose a reason for hiding this comment

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

Should be fixed now in aba596f

Copy link
Member

@ellatrix ellatrix Jun 4, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Hm 🤔 Making a selection doesn't seem like the sort of action you take when you want to insert something. If anything, that would mean, delete the selection and insert something instead, but then I'd rather have it explicitly separate.

The inspector toggle is currently hidden when the multi-select header takes over, not sure if we want to keep something there, but both the inserter and inspector would then be actions tied to multi-selection and would not remove the selection on click.

To me is seems better that clicking or moving focus anywhere outside the UI that has to do with multi-selection should deselect.

Copy link
Member

@ellatrix ellatrix Jun 4, 2017

Choose a reason for hiding this comment

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

I'd think of the multi-select header as having tabIndex="-1" and being some weird kind of modal. It would contain the focus, and if you click outside of it, or press escape, the selection is cleared, just like a modal would be closed. As long as the selection is there, backspace always deletes the selection content.

Copy link
Contributor

@jasmussen jasmussen Jun 5, 2017

Choose a reason for hiding this comment

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

Does this need a decision somehow? I'm a bit confused on the details, but I'd like to help work through the flow if need be, and if someone can clarify the conflict.

Copy link
Contributor Author

@youknowriad youknowriad Jun 5, 2017

Choose a reason for hiding this comment

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

Yep we definitely need a decision here. So for now on master, we're considering the browser selection (and focus) as separate from the selectedBlock and multiSelectedBlocks (which should probably be merged into one unique concept).

We're doing so because we want to be able to open the inserter and the sidebar without losing the selection (note that clicking buttons moves the focus to the button).

But multi-selection allows us to delete the selected blocks by hitting "backspace" which has no sense if the selection is not focused (backspace should work on the focused input or text area ...)

also Ella is saying

The inspector toggle is currently hidden when the multi-select header takes over

This was not the case when this PR was merged.

So should we tie the selection to the focus which means clicking anywhere deselect everything or should we find a middle-ground where we do so only for multi-selection?

Copy link
Contributor

@jasmussen jasmussen Jun 5, 2017

Choose a reason for hiding this comment

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

This is complex, and I think I'm not getting all the nuance, so please correct me if I'm wrong here — it seems like we have two types of selection:

  1. selection when you click the block
  2. selection when a block is selected via multi select

1 is important for the inspector, as we need to be able to adjust parameters there without deselecting the block. So far so good.

But multi-selection allows us to delete the selected blocks by hitting "backspace" which has no sense if the selection is not focused (backspace should work on the focused input or text area ...)

I feel very daft here, the behavior in master feels pretty okay with me:

  • I select across blocks, press backspace, and the blocks are deleted
  • I select ... oh I think I see what you're getting at.

If I click inside a text block and press backspace, I delete text. If I then click on the border, the text is unfocused and it seems like the block is focused. Then backspace deletes the block. This is especially important, I think, for the bigger blocks, like image, where you almost always select the block.

If I understand you correctly, the big challenge here is that there's confusion between selecting blocks, setting focus on text, and selecting across blocks, correct?

It seems like you're saying selecting a block (clicking on the border), should highlight the block the same way as multi select does, because it's technically almost the same? Let's imagine what would happen if we did that — would this be desirable or not? Hmm. Thinking.

)( VisualEditorBlockList );
10 changes: 1 addition & 9 deletions editor/modes/visual-editor/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ class VisualEditorBlock extends wp.element.Component {
this.maybeStartTyping = this.maybeStartTyping.bind( this );
this.removeOrDeselect = this.removeOrDeselect.bind( this );
this.mergeBlocks = this.mergeBlocks.bind( this );
this.selectAndStopPropagation = this.selectAndStopPropagation.bind( this );
this.previousOffset = null;
}

Expand Down Expand Up @@ -151,13 +150,6 @@ class VisualEditorBlock extends wp.element.Component {
}
}

selectAndStopPropagation( event ) {
this.props.onSelect();
// Visual editor infers click as intent to clear the selected block, so
// prevent bubbling when occurring on block where selection is intended
event.stopPropagation();
}

componentDidUpdate( prevProps ) {
if ( this.previousOffset ) {
window.scrollTo(
Expand Down Expand Up @@ -280,7 +272,7 @@ class VisualEditorBlock extends wp.element.Component {
<div
onKeyPress={ this.maybeStartTyping }
onFocus={ onSelect }
onClick={ this.selectAndStopPropagation }
onClick={ onSelect }
>
<BlockEdit
focus={ focus }
Expand Down
62 changes: 50 additions & 12 deletions editor/modes/visual-editor/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
/**
* External dependencies
*/
import { connect } from 'react-redux';

/**
* WordPress dependencies
*/
import { __ } from 'i18n';
import { Component } from 'element';

/**
* Internal dependencies
Expand All @@ -11,16 +17,48 @@ import Inserter from '../../inserter';
import VisualEditorBlockList from './block-list';
import PostTitle from '../../post-title';

export default function VisualEditor() {
return (
<div
role="region"
aria-label={ __( 'Visual Editor' ) }
className="editor-visual-editor"
>
<PostTitle />
<VisualEditorBlockList />
<Inserter position="top right" />
</div>
);
class VisualEditor extends Component {
constructor() {
super( ...arguments );
this.bindContainer = this.bindContainer.bind( this );
this.onClick = this.onClick.bind( this );
}

bindContainer( ref ) {
this.container = ref;
}

onClick( event ) {
if ( event.target === this.container ) {
Copy link
Member

@aduth aduth Jun 2, 2017

Choose a reason for hiding this comment

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

Between this and #836, I think I like this approach better. But it also means we probably don't need the stopPropagation anymore.

Copy link
Contributor Author

@youknowriad youknowriad Jun 2, 2017

Choose a reason for hiding this comment

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

updating

this.props.clearSelectedBlock();
}
}

render() {
// Disable reason: Clicking the canvas should clear the selection
/* eslint-disable jsx-a11y/no-static-element-interactions, jsx-a11y/onclick-has-role, jsx-a11y/click-events-have-key-events */
return (
<div
role="region"
aria-label={ __( 'Visual Editor' ) }
className="editor-visual-editor"
onClick={ this.onClick }
ref={ this.bindContainer }
>
<PostTitle />
<VisualEditorBlockList />
<Inserter position="top right" />
</div>
);
/* eslint-enable jsx-a11y/no-static-element-interactions, jsx-a11y/onclick-has-role, jsx-a11y/click-events-have-key-events */
}
}

export default connect(
undefined,
( dispatch ) => ( {
clearSelectedBlock() {
dispatch( { type: 'CLEAR_SELECTED_BLOCK' } );
},
} )
)( VisualEditor );
7 changes: 4 additions & 3 deletions editor/modes/visual-editor/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,15 +125,16 @@
margin: $item-spacing $item-spacing $item-spacing calc( 50% - #{ $visual-editor-max-width / 2 } ); // account for full-width trick
}

.editor-visual-editor > .editor-visual-editor__insertion-point {
.editor-visual-editor .editor-visual-editor__insertion-point {
position: relative;
margin-right: #{ ( $block-padding + $block-mover-margin) * 2 };

&:before {
position: absolute;
top: 6px;
right: #{ $block-padding + $block-mover-margin };
left: 0;
height: 3px;
left: 0;
right: 0;
background: $blue-medium-500;
content: '';
}
Expand Down
1 change: 1 addition & 0 deletions editor/state.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,7 @@ export function multiSelectedBlocks( state = { start: null, end: null }, action
switch ( action.type ) {
case 'CLEAR_SELECTED_BLOCK':
case 'TOGGLE_BLOCK_SELECTED':
case 'INSERT_BLOCK':
return {
start: null,
end: null,
Expand Down
10 changes: 10 additions & 0 deletions editor/test/state.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -786,6 +786,16 @@ describe( 'state', () => {
} );

expect( state2 ).to.eql( { start: null, end: null } );

const state3 = multiSelectedBlocks( original, {
type: 'INSERT_BLOCK',
block: {
uid: 'ribs',
name: 'core/freeform',
},
} );

expect( state3 ).to.eql( { start: null, end: null } );
} );
} );

Expand Down