Visual Editor: Keep the block at the same position while moving it #463
Conversation
youknowriad
added
the
Framework
|
Works great! Thumbs up from a UX perspective 👍 |
editor/modes/visual-editor/block.js
Outdated
| class VisualEditorBlock extends wp.element.Component { | ||
| constructor() { | ||
| super( ...arguments ); | ||
| this.bindBlockNode = this.bindBlockNode.bind( this ); |
There was a problem hiding this comment.
Does this function need to be bound?
editor/modes/visual-editor/block.js
Outdated
| window.scrollX, | ||
| window.scrollY + this.node.getBoundingClientRect().top - this.previousOffset | ||
| ); | ||
| this.previousOffset = false; |
There was a problem hiding this comment.
I'm not a fan of using false as an empty state. Could we assign to null, undefined, or even delete this.previousOffset instead?
There was a problem hiding this comment.
Sounds good, do you have a preference for one over another? delete ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'll go for initialization :)
| this.props.isSelected && | ||
| newProps.isSelected | ||
| ) { | ||
| this.previousOffset = this.node.getBoundingClientRect().top; |
There was a problem hiding this comment.
Would it have been possible to simply store window.scrollY here and restore it directly in the DidUpdate?
There was a problem hiding this comment.
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.
editor/modes/visual-editor/block.js
Outdated
| onDeselect(); | ||
| const { onChange, onSelect, onDeselect, onMouseEnter, onMouseLeave, onInsertAfter } = this.props; | ||
|
|
||
| function setAttributes( attributes ) { |
There was a problem hiding this comment.
If we're making this a proper class, these might ought to be proper member functions now.
editor/modes/visual-editor/block.js
Outdated
| } | ||
|
|
||
| export default connect( | ||
| ( state, ownProps ) => ( { | ||
| order: state.blocks.order.findIndex( uid => ownProps.uid === uid ), |
There was a problem hiding this comment.
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 )
closes #161
Nothing too fancy here. I'm using the lifecycle methods to save the scroll position before the update and restore it after.