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

Editor: Fix the scroll position when reordering blocks #2953

Merged
merged 1 commit into from Oct 10, 2017

Conversation

Projects
None yet
4 participants
@youknowriad
Contributor

youknowriad commented Oct 10, 2017

closes #2936

This regressed when we changed the scrollable area (from the global window to the editor's layout).
The fix here scroll's the editor's layout instead.

Caveats

  • A selector to the editor's layout node is hard-coded in the Block.js file. This is not ideal, should we provide this node as prop? Should this be something in the editor's settings?

Teseting instructions

  • Try moving a block
  • The scroll position should be adjusted to keep the block at the same position

@youknowriad youknowriad self-assigned this Oct 10, 2017

@youknowriad youknowriad requested review from aduth and iseulde Oct 10, 2017

@@ -87,6 +87,9 @@ class VisualEditorBlock extends Component {
if ( this.props.isTyping ) {
document.addEventListener( 'mousemove', this.stopTypingOnMouseMove );
}
// Not Ideal, but it's the easiest way to get the scrollable container
this.editorLayout = document.querySelector( '.editor-layout__editor' );

This comment has been minimized.

@mtias

mtias Oct 10, 2017

Contributor

Any other ideas here?

@mtias

mtias Oct 10, 2017

Contributor

Any other ideas here?

This comment has been minimized.

@iseulde

iseulde Oct 10, 2017

Member

Multi-select also broke because of the same scroll container change. I've changed that so it's independent of the scroll position, but in this case you need the container to do a scroll... Maybe better to pass the container down as a prop?

@iseulde

iseulde Oct 10, 2017

Member

Multi-select also broke because of the same scroll container change. I've changed that so it's independent of the scroll position, but in this case you need the container to do a scroll... Maybe better to pass the container down as a prop?

This comment has been minimized.

@iseulde

iseulde Oct 10, 2017

Member

We can use the prop in block-list too then, and avoid calculating the dimension on scroll and mousemove.

@iseulde

iseulde Oct 10, 2017

Member

We can use the prop in block-list too then, and avoid calculating the dimension on scroll and mousemove.

This comment has been minimized.

@aduth

aduth Oct 10, 2017

Member

Popover achieved this (specifying DOM target) with a context provider, though I'm seeking to move away from it in favor of Slot-Fill:

https://github.com/WordPress/gutenberg/tree/master/components/popover#usage

@aduth

aduth Oct 10, 2017

Member

Popover achieved this (specifying DOM target) with a context provider, though I'm seeking to move away from it in favor of Slot-Fill:

https://github.com/WordPress/gutenberg/tree/master/components/popover#usage

@mtias mtias added the Chrome label Oct 10, 2017

@aduth

aduth approved these changes Oct 10, 2017 edited

Re: Explicitly referencing the DOM node, could use a blaring TODO: Not this (or associated issue) but if we want this fixed in the next release, this could be a good interim solution.

@youknowriad youknowriad merged commit af4dc3a into master Oct 10, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@youknowriad youknowriad deleted the fix/scrolling-when-ordering branch Oct 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment