Allow drag to different page #3312

barklund · 2019-09-20T12:23:02Z

Fixes #3311.

This PR allows editors to drag elements to other pages.

See early demo here: https://www.youtube.com/watch?v=ONFOvRF2jog

This is done by while dragging, detecting where the actual mouse cursor is relative to the current page. A highlight callback is invoked while dragging with the offset of the page currently under the cursor. If dropped above another page, a drop callback is invoked.

The callbacks then check if there even is a page in this direction and then either highlight that page or clone the element to that page respectively.

TODO

Allow dragging an element to another page.
Fix dragging for CTAs.
Figure out if we need dragging for Page Attachments (resolution: we don't for now).
Maintain proper Separation of Concerns.
Make sure CTA can't be moved to a page which already has a CTA.
Tests? Unit testing is almost impossible, but an e2e test is feasible.

_Fixes #3311._ This WIP allows editors to drag elements to neighboring pages. This is done by while dragging, detecting where the actual mouse cursor is relative to the current page. A highlight callback is invoked while dragging with the offset of the page currently under the cursor. If dropped above a neighboring page, a drop callback is invoked. The callbacks then check if there even is a neigbor in this direction and then either highlight that page or clone the element to that page. It doesn't work correctly for CTA's and Page Attachments yet. The code is also a little weirdly structured with the bulk of the logic placed in `block-draggable`, which doesn't really sound like the right place - concern-wise. Perhaps a simple renaming solves it, but perhaps a larger restructuring is in order.

assets/src/stories-editor/components/block-mover/block-draggable.js

CTA blocks have their positions defined by a different property than regular blocks, 'btnPositionLeft` rather than `positionLeft` and similarly for top. This fixes that. Also, CTA blocks aren't allowed on the first page. This invariant is also maintained now.

assets/src/stories-editor/components/block-mover/block-draggable.js

assets/src/stories-editor/components/block-mover/draggable.js

spacedmonkey · 2019-09-25T18:00:02Z

This PR really needs e2e tests.

I would also look rebasing and running those e2e tests against AXE for accessibility. I would look aria-grabbed and aria-dropeffect accessibility attributes. These attributes will likely need to be updated dynamically blocks move.

spacedmonkey · 2019-09-25T18:22:33Z

We might want to update using the screen reader feature using speak. We might want to update these users that the drag was / wasn't successful.

barklund · 2019-09-26T13:12:33Z

This PR really needs e2e tests.

I'm having problems getting the e2e setup to properly test these. It might be because those tests are just too power-intensive for my machine though (which is bad!).

Also, e2e's aren't meant for exhaustive testing in any way. It's an anti-pattern if e2e's are used for this. e2e should only be used for some very overall happy-path testing to ensure, that overall dragging works as expected. Unit and integration testing should be done to validate the finer things.

swissspidy · 2019-09-26T13:13:23Z

FWIW we've had issues in the past with testing dragging as Chromium/Puppeteer doesn't have the best support there.

barklund · 2019-09-26T13:15:54Z

We might want to update using the screen reader feature using speak. We might want to update these users that the drag was / wasn't successful.

Uh, that's a simple yet interesting package. It could definitely be used here. But maybe the overall a11y of dragging should be considered in some strategy rather than hotfixes I can attempt with speak(). I don't have the knowledge required to make sure this is actually accessible and doesn't just feel like it.

spacedmonkey · 2019-09-26T14:17:30Z

I don't have the knowledge required to make sure this is actually accessible and doesn't just feel like it.

I agree here, my understanding of speak in this use case is limited. Not sure what exactly the messaging should be here. Maybe we could loop in an accessibility expect for some help here.

However the aria-grabbed and aria-dropeffect accessibility attributes seem simple enough to implement here and would be a good start.

This feature is not an accessibility friendly one in the first place, as most people with accessibility needs will be using screen readers or keyboard anyway. Which is why #3330 would be better for those users.

Several locations checked if a block was allowed on a given page, including the media inserter, the shortcut handler, paste handler and now also multi-page drag handler. All these instances have now been a) centralised and b) changed to use the built-in method of checking for allowed blocks inside another block using core block editor functionality. This also removed the need for a specific test for the `ensureAllowedBlocksOnPaste` as it just used other functions now more specific and central to the issue.

barklund · 2019-09-26T17:31:48Z

The recent refactor of the "is block A allowed on page B" functionality touches on a lot of extra components, that will need to be QA'ed as well: Media Inserter, Shortcuts, Paste Block.

Just FYI for future reference.

assets/src/stories-editor/components/inserter/menu.js

assets/src/stories-editor/helpers/useMoveBlockToPage.js

spacedmonkey

Really great work on the refactor to use hooks. The code is much clearer.
Just a couple of tweaks, questions and comments to be added. Otherwise, we are looking good 👍

Work noting that they are conflicts that need to be resolved.

This converts the helper function `isBlockAllowedOnPage` to the hook `useIsBlockAllowedOnPage` which ensures that selectors are properly updated once the registry changes and any component using it is properly re-rendered. However, because some uses of `isBlockAllowedOnPage` is located in class components, the old function has only been deprecated for now, not completely removed.

Making `attributes` optional and adding unit tests for all aspects of the hook.

swissspidy · 2019-10-04T13:49:36Z

@spacedmonkey Could you re-review this?

spacedmonkey · 2019-10-04T13:59:22Z

assets/src/stories-editor/components/inserter/menu.js

@@ -52,6 +52,7 @@ import { addQueryArgs } from '@wordpress/url';
 import BlockPreview from '../block-preview';
 import BlockTypesList from '../block-types-list';
 import { ALLOWED_TOP_LEVEL_BLOCKS } from '../../constants';
+import { isBlockAllowedOnPage } from '../../helpers';


Why can't the hook be used here?

Because this is still a class component, not a functional one. See #2538 for explanation.

I am just worried about bug coming from not using select correctly here and getting stale data.

Isn't the benefit of hooks if that you put them everyone and mix and match?

How that isBlockAllowedOnPage is only used in Menu, I think we should get rid of the helper and move the logic to withSelect

That could work yeah 👍

assets/src/stories-editor/components/media-inserter/index.js

spacedmonkey · 2019-10-04T14:17:46Z

This bug is still happening for me on the latest update.
See screencast. https://www.youtube.com/watch?v=0ZQDFQVPRYw&feature=youtu.be

swissspidy · 2019-10-04T14:21:18Z

This bug is still happening for me on the latest update.

Hmm that seems to be because the drop zone around a page is a bit bigger than the page itself, to allow for better positioning of elements.

This way it can use the new `useIsBlockAllowedOnPage` hook

assets/src/stories-editor/components/media-inserter/index.js

barklund · 2019-10-04T15:12:20Z

This bug is still happening for me on the latest update.
See screencast. https://www.youtube.com/watch?v=0ZQDFQVPRYw&feature=youtu.be

So, the thing is, you're allowed to drop items off-page if you feel like it – but only within a certain boundary off-page. The general item drop is handled in story-block-drop-zone, and I actually think the size of the boundary is determined by the size of an element, thus via CSS - but not sure about that.

However, in this case it gets extra weird, because off-page is technically on top of another page, that you just can't move the item to. I don't know if there's an easy way to handle this, that doesn't impact too many other things and doesn't feel like a workaround and special-case-handling.

I'll look into this drop zone, and maybe limit the drop zone so it never overlaps a neighbouring page - seems like a fair restriction.

assets/src/stories-editor/components/media-inserter/index.js

Remove last remaining usage of that function as all other components use the new `useIsBlockAllowedOnPage` hook.

spacedmonkey

LGTM

swissspidy · 2019-10-04T15:58:10Z

Given that the situation with the drop zone mentioned above has been around before, it can be addressed in a separate PR

barklund added Enhancement New feature or improvement of an existing one AMP Stories labels Sep 20, 2019

barklund requested review from spacedmonkey, dero, swissspidy and miina September 20, 2019 12:23

barklund self-assigned this Sep 20, 2019

googlebot added the cla: yes Signed the Google CLA label Sep 20, 2019

Refactor duplicated logic

ba20251

swissspidy reviewed Sep 20, 2019

View reviewed changes

assets/src/stories-editor/components/block-mover/block-draggable.js Outdated Show resolved Hide resolved

barklund added 8 commits September 20, 2019 21:06

Remove unnecessary hook

354443b

More performant structure

40259f8

Better separation of concerns

9fd43c3

Fix proptypes and add inline comment

f7d0c19

Fix offset bug when dragging 2+ pages left

83b2e4f

Adding documentation for functions

3928f60

Fix disallow duplicate CTA

efd22fd

spacedmonkey suggested changes Sep 25, 2019

View reviewed changes

barklund commented Sep 26, 2019

View reviewed changes

assets/src/stories-editor/components/inserter/menu.js Show resolved Hide resolved

spacedmonkey reviewed Oct 3, 2019

View reviewed changes

assets/src/stories-editor/helpers/useMoveBlockToPage.js Outdated Show resolved Hide resolved

spacedmonkey suggested changes Oct 3, 2019

View reviewed changes

barklund added 5 commits October 3, 2019 11:18

Merge branch 'develop' into feature/3311-allow-drag-to-page

3f13272

Added dependencies for useSelect

3241e3b

Created new constant for the height of the drop zone for CTAs

005bac0

Updated useMoveBlockToPage and added tests

354ee52

Making `attributes` optional and adding unit tests for all aspects of the hook.

barklund requested a review from spacedmonkey October 3, 2019 16:56

Fix proptype missed in refactor

8f068be

spacedmonkey mentioned this pull request Oct 4, 2019

Snapping Guidelines #3060

Merged

18 tasks

Merge branch 'develop' into feature/3311-allow-drag-to-page

d5eb6a8

swissspidy approved these changes Oct 4, 2019

View reviewed changes

spacedmonkey reviewed Oct 4, 2019

View reviewed changes

assets/src/stories-editor/components/media-inserter/index.js Outdated Show resolved Hide resolved

Refactor MediaInserter to use hooks

144c430

This way it can use the new `useIsBlockAllowedOnPage` hook

spacedmonkey reviewed Oct 4, 2019

View reviewed changes

assets/src/stories-editor/components/media-inserter/index.js Show resolved Hide resolved

barklund commented Oct 4, 2019

View reviewed changes

assets/src/stories-editor/components/media-inserter/index.js Outdated Show resolved Hide resolved

swissspidy added 2 commits October 4, 2019 17:29

Move isBlockAllowedOnPage function inline

60f0198

Remove last remaining usage of that function as all other components use the new `useIsBlockAllowedOnPage` hook.

Remove ifCondition HOC usage from stories editor

9bd9ab3

spacedmonkey approved these changes Oct 4, 2019

View reviewed changes

swissspidy added this to the v1.3.1 milestone Oct 4, 2019

swissspidy merged commit b7b5539 into develop Oct 4, 2019

swissspidy deleted the feature/3311-allow-drag-to-page branch October 4, 2019 15:59

swissspidy modified the milestones: v1.3.1, v1.4 Oct 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow drag to different page #3312

Allow drag to different page #3312

barklund commented Sep 20, 2019 •

edited

Loading

spacedmonkey commented Sep 25, 2019

spacedmonkey commented Sep 25, 2019

barklund commented Sep 26, 2019

swissspidy commented Sep 26, 2019

barklund commented Sep 26, 2019

spacedmonkey commented Sep 26, 2019

barklund commented Sep 26, 2019

spacedmonkey left a comment •

edited

Loading

swissspidy commented Oct 4, 2019

spacedmonkey Oct 4, 2019

swissspidy Oct 4, 2019

spacedmonkey Oct 4, 2019

spacedmonkey Oct 4, 2019

spacedmonkey Oct 4, 2019

swissspidy Oct 4, 2019

spacedmonkey commented Oct 4, 2019

swissspidy commented Oct 4, 2019

barklund commented Oct 4, 2019

spacedmonkey left a comment

swissspidy commented Oct 4, 2019

Allow drag to different page #3312

Allow drag to different page #3312

Conversation

barklund commented Sep 20, 2019 • edited Loading

TODO

spacedmonkey commented Sep 25, 2019

spacedmonkey commented Sep 25, 2019

barklund commented Sep 26, 2019

swissspidy commented Sep 26, 2019

barklund commented Sep 26, 2019

spacedmonkey commented Sep 26, 2019

barklund commented Sep 26, 2019

spacedmonkey left a comment • edited Loading

Choose a reason for hiding this comment

swissspidy commented Oct 4, 2019

spacedmonkey Oct 4, 2019

Choose a reason for hiding this comment

swissspidy Oct 4, 2019

Choose a reason for hiding this comment

spacedmonkey Oct 4, 2019

Choose a reason for hiding this comment

spacedmonkey Oct 4, 2019

Choose a reason for hiding this comment

spacedmonkey Oct 4, 2019

Choose a reason for hiding this comment

swissspidy Oct 4, 2019

Choose a reason for hiding this comment

spacedmonkey commented Oct 4, 2019

swissspidy commented Oct 4, 2019

barklund commented Oct 4, 2019

spacedmonkey left a comment

Choose a reason for hiding this comment

swissspidy commented Oct 4, 2019

barklund commented Sep 20, 2019 •

edited

Loading

spacedmonkey left a comment •

edited

Loading