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.

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

Visual Editor: Keep the block at the same position while moving it #463

Merged
merged 2 commits into from
Apr 20, 2017
Merged

Visual Editor: Keep the block at the same position while moving it #463

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
be4b87c
Visual Editor: Keep the block at the same position while moving it
youknowriad Apr 20, 2017
0c66b8c
Initialize property + using indexOf + moving functions
youknowriad Apr 20, 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
154 changes: 93 additions & 61 deletions editor/modes/visual-editor/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,84 +11,116 @@ import Toolbar from 'components/toolbar';
import BlockMover from 'components/block-mover';
import BlockSwitcher from 'components/block-switcher';

function VisualEditorBlock( props ) {
const { block } = props;
const settings = wp.blocks.getBlockSettings( block.blockType );
class VisualEditorBlock extends wp.element.Component {
constructor() {
super( ...arguments );
this.bindBlockNode = this.bindBlockNode.bind( this );
Copy link
Member

Choose a reason for hiding this comment

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

Does this function need to be bound?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes?

}

let BlockEdit;
if ( settings ) {
BlockEdit = settings.edit || settings.save;
bindBlockNode( node ) {
this.node = node;
}

if ( ! BlockEdit ) {
return null;
componentWillReceiveProps( newProps ) {
if (
this.props.order !== newProps.order &&
this.props.isSelected &&
newProps.isSelected
) {
this.previousOffset = this.node.getBoundingClientRect().top;
Copy link
Member

Choose a reason for hiding this comment

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

Would it have been possible to simply store window.scrollY here and restore it directly in the DidUpdate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, actually we don't want to keep the same scrollY since the block is moving, we want the block to show in the same position.

}
}

const { isHovered } = props;
const isSelected = props.isSelected;
const className = classnames( 'editor-visual-editor__block', {
'is-selected': isSelected,
'is-hovered': isHovered
} );
componentDidUpdate() {
if ( this.previousOffset ) {
window.scrollTo(
window.scrollX,
window.scrollY + this.node.getBoundingClientRect().top - this.previousOffset
);
this.previousOffset = false;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a fan of using false as an empty state. Could we assign to null, undefined, or even delete this.previousOffset instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, do you have a preference for one over another? delete ?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good, do you have a preference for one over another? delete ?

Sorta ties into a question on whether we want to be extremely clear about the existence of the instance property by initializing it either in the constructor or as an instance initializer (I don't recall if this is a spec feature or one of the staged ones).

As it is here without initialization, I'd say delete is most appropriate.
Otherwise, if we were to initialize, probably initialize to null and reset to null here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll go for initialization :)

}
}

const { onChange, onSelect, onDeselect, onMouseEnter, onMouseLeave, onInsertAfter } = props;
render() {
const { block } = this.props;
const settings = wp.blocks.getBlockSettings( block.blockType );

function setAttributes( attributes ) {
onChange( {
attributes: {
...block.attributes,
...attributes
}
let BlockEdit;
if ( settings ) {
BlockEdit = settings.edit || settings.save;
}

if ( ! BlockEdit ) {
return null;
}

const { isHovered, isSelected } = this.props;
const className = classnames( 'editor-visual-editor__block', {
'is-selected': isSelected,
'is-hovered': isHovered
} );
}

function maybeDeselect( event ) {
// Annoyingly React does not support focusOut and we're forced to check
// related target to ensure it's not a child when blur fires.
if ( ! event.currentTarget.contains( event.relatedTarget ) ) {
onDeselect();
const { onChange, onSelect, onDeselect, onMouseEnter, onMouseLeave, onInsertAfter } = this.props;

function setAttributes( attributes ) {
Copy link
Member

Choose a reason for hiding this comment

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

If we're making this a proper class, these might ought to be proper member functions now.

onChange( {
attributes: {
...block.attributes,
...attributes
}
} );
}
}

// Disable reason: Each block can receive focus but must be able to contain
// block children. Tab keyboard navigation enabled by tabIndex assignment.
function maybeDeselect( event ) {
// Annoyingly React does not support focusOut and we're forced to check
// related target to ensure it's not a child when blur fires.
if ( ! event.currentTarget.contains( event.relatedTarget ) ) {
onDeselect();
}
}

/* eslint-disable jsx-a11y/no-static-element-interactions */
return (
<div
tabIndex="0"
onFocus={ onSelect }
onBlur={ maybeDeselect }
onKeyDown={ onDeselect }
onMouseEnter={ onMouseEnter }
onMouseLeave={ onMouseLeave }
className={ className }
>
{ ( isSelected || isHovered ) && <BlockMover uid={ block.uid } /> }
<div className="editor-visual-editor__block-controls">
{ isSelected && <BlockSwitcher uid={ block.uid } /> }
{ isSelected && settings.controls ? (
<Toolbar
controls={ settings.controls.map( ( control ) => ( {
...control,
onClick: () => control.onClick( block.attributes, setAttributes ),
isActive: () => control.isActive( block.attributes )
} ) ) } />
) : null }
// Disable reason: Each block can receive focus but must be able to contain
// block children. Tab keyboard navigation enabled by tabIndex assignment.

/* eslint-disable jsx-a11y/no-static-element-interactions */
return (
<div
ref={ this.bindBlockNode }
tabIndex="0"
onFocus={ onSelect }
onBlur={ maybeDeselect }
onKeyDown={ onDeselect }
onMouseEnter={ onMouseEnter }
onMouseLeave={ onMouseLeave }
className={ className }
>
{ ( isSelected || isHovered ) && <BlockMover uid={ block.uid } /> }
<div className="editor-visual-editor__block-controls">
{ isSelected && <BlockSwitcher uid={ block.uid } /> }
{ isSelected && settings.controls ? (
<Toolbar
controls={ settings.controls.map( ( control ) => ( {
...control,
onClick: () => control.onClick( block.attributes, setAttributes ),
isActive: () => control.isActive( block.attributes )
} ) ) } />
) : null }
</div>
<BlockEdit
isSelected={ isSelected }
attributes={ block.attributes }
setAttributes={ setAttributes }
insertBlockAfter={ onInsertAfter }
/>
</div>
<BlockEdit
isSelected={ isSelected }
attributes={ block.attributes }
setAttributes={ setAttributes }
insertBlockAfter={ onInsertAfter }
/>
</div>
);
/* eslint-enable jsx-a11y/no-static-element-interactions */
);
/* eslint-enable jsx-a11y/no-static-element-interactions */
}
}

export default connect(
( state, ownProps ) => ( {
order: state.blocks.order.findIndex( uid => ownProps.uid === uid ),
Copy link
Member

Choose a reason for hiding this comment

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

We haven't enabled the polyfill, so this will error in IE11. We might just consider enabling the polyfill, but in this case couldn't this just be:

order: state.blocks.order.indexOf( ownProps.uid )

block: state.blocks.byUid[ ownProps.uid ],
isSelected: state.selectedBlock === ownProps.uid,
isHovered: state.hoveredBlock === ownProps.uid
Expand Down