Allow dragging entire blocks by kienstra · Pull Request #2389 · ampproject/amp-wp

kienstra · 2019-05-23T07:37:54Z

Removes the drag handle, and usually allows dragging the entire block
Dragging usually doesn't work well when pointer: cursor, especially in the Text block

Update: dragging the image block works better now

This has several bugs, and might not be the approach that we go with. But you can now drag the entire block for some blocks, like text and "meta" blocks.

There were several errors, so address those here.

The only conflict was in: with-amp-story-settings.js Retain edits in both branches.

Much of the styling in edit.css applied to the drag handle, which is removed.

On dragging, the browser creates an image of the target, for example, the entire text block. But there's already a clone below that's rotated in case the block is rotated, and this can create a non-rotated duplicate of that. So override this with an empty image.

It looks like images normally appear to drag in HTML, though not all images can drop. This caused a duplicate image to appear. So set the image to *-user-drag: none. This isn't the last bugfix needed for dragging the image block.

Before, the <StoryBlockMover> wrapped almost the entire markup of the edit component. This seemed to create issues where resizing seemed to also drag. So instead, nest the <StoryBlockMover> inside the rotating and resizing elements.

Some files were modified in the latest commits, so note that here.

kienstra · 2019-05-27T02:42:31Z

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

+		 * So override this with an empty image.
+		 */
+		const dragImage = new Image();
+		event.dataTransfer.setDragImage( dragImage, 0, 0 );


This workaround might need more thought, but at least it prevents a duplicate, non-rotated image on dragging.

Without this workaround (maybe hack):

kienstra · 2019-05-27T02:59:01Z

Known Issues

Pending approval of this PR's approach, I could work on these issues:

On clicking the image block and dragging it slowly, it sometimes engages the <DropZone> of the <MediaUpload> from the image block, not that of the AMP Story page block. This doesn't always happen, though:
It's hard to drag the text block. It mainly drags on the edges of the block:

kienstra · 2019-05-27T03:19:21Z

Request For Review

Hi @swissspidy and @miina,
Hope you're doing great. Could you please review the approach in this PR?

There are known issues above, and possibly more bugs. I could work on those if you agree on the overall approach here.

Overall, dragging the entire block is working pretty well. The main exception I found was the text block.

Thanks!

kienstra · 2019-05-27T03:25:50Z

assets/src/components/block-mover/block-drag-area.js

 						draggable
 					>
-						{ icon }
+						{ children }


This <BlockDragArea> now wraps the block (children), instead of appearing alongside it. This allows dragging the entire block.

miina · 2019-05-27T10:59:16Z

@kienstra It might be a good thing that text block is only draggable from edges, this will keep the functionality of selecting text for bold and italic. We might just need to figure out how to have the draggable area larger so that mainly only the area covered by Text wouldn't be draggable.

miina · 2019-05-27T13:40:27Z

@kienstra Looks like there is very few room between the edges of the Text block input and the edge of the block, we should probably figure out a way which would be the best option for keeping the bold and italic functionality as it is right now, and at the same time leaving more room for dragging. Perhaps just adding some kind of padding to the block (not the input) could work.

A few other notes:

The dropzone (handled via CSS) was previously considering the drag handle only, for example, if you add an image and drag it from it's right edge out of the right side of the page, it goes out of the drag zone and thus doesn't drag. We might need to extend the drag zone.
We should probably display drag handle at the areas where the block is draggable, especially in case of a Text block.

Generally the approach looks good to me!

swissspidy · 2019-05-27T13:40:26Z

assets/src/components/resizable-box/edit.css

 	height: 100%;
 	object-fit: cover;
+	/* Prevents dragging the image itself, as its container is draggable. */
+	-webkit-user-drag: none;


Pretty sure that only -webkit-user-drag exists nowadays. The other properties either never existed (because it was never in the spec) or are simply irrelevant.

Thanks, good point.

cae00b7 removes these properties. You're right, I can't find any reference to them with quick searches.

swissspidy · 2019-05-27T14:36:27Z

We should probably display drag handle at the areas where the block is draggable

cursor: grab would be good here.

Regarding movable text blocks, I feel like we should try a different route here.

While in a regular post it makes sense to be able to select and format some text without selecting a block first, I don't think that necessarily applies to the stories editor as well.

My immediate suggestion after comparing with other tools like Snapchat and Instagram would be this:

If the text block is not selected, then I should be able to just drag it anywhere
If I select the text block, then I should be able to just drag it anywhere
If the text block is selected, then I should be able to click on the text block again in order to write, edit, select the text in it.

This is kinda similar to what's currently being worked on upstream in WordPress/gutenberg#15537, where an inner block is only selected after selecting the parent block first.

tl;dr: First click -> select block, dragging possible. Second click -> edit text, no dragging

With that being said, before we add any changes here, I want everyone to try the current state of this PR first. Then we can incorporate this feedback appropriately.

cc @ThierryA @amedina @westonruter

ThierryA · 2019-05-27T15:03:49Z

@swissspidy I think text block dragging behaviour should as implemented in this PR with the addition of cursor: grab.

i.e. I don't think users should have to click twice in order to edit text as you suggested (click to select element + click to edit).

As Pascal mentioned, it looks like these don't actually exist.

As suggested on the PR, add this, but change it back to the default for the editable area of text on the Text block

kienstra · 2019-05-28T11:29:15Z

Hi @miina,
Thanks for reviewing this. Sorry, I accidentally replied to your comment with a confused emoji, I meant to choose the 😄

We should probably display drag handle at the areas where the block is draggable, especially in case of a Text block.

Good idea, ef75b10 shows the drag handle.

I couldn't figure out a way to add padding within the Text block, so that the drag handle would display more.

For example, with:

.wp-block[data-type="amp/amp-story-text"] [draggable="true"] {
	padding: 5px;
}

the front-end display is a little too far to the left:

The dropzone (handled via CSS) was previously considering the drag handle only, for example, if you add an image and drag it from it's right edge out of the right side of the page, it goes out of the drag zone and thus doesn't drag. We might need to extend the drag zone.

Thanks, I didn't notice that before. I'll work on extending the drag zone.

kienstra · 2019-05-28T11:32:29Z

Hi @swissspidy,
Thanks a lot for your review here.

cursor: grab would be good here.

Thanks, ef75b10 uses your idea of cursor: grab

As Miina mentioned, this was originally intended to be only for the handle on the left. Now, dragging from the right of a block is more limited. Still, I'm not sure if this is the right amount to change it, or if this is the right value to change.

Ensures that inner elements of meta blocks always use the full height. That way one use the full occupied area for dragging and text is propperly vertically centered when using amp-fit-text.

googlebot · 2019-06-10T11:08:43Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

swissspidy

I think this is a good solution for now.

miina · 2019-06-10T11:51:02Z

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

+	 */
+	componentDidMount() {
+		document.querySelectorAll( '.block-editor-block-list__block[data-type="core/image"] img' ).forEach( ( image ) => {
+			image.setAttribute( 'draggable', 'false' );


Not critical but I wonder if there's a better place for doing this -- it seems to do the same for every Draggable and each time for all the images.

miina · 2019-06-10T12:11:28Z

Some notes from testing:

List block: The mouse pointer is displayed as drag handle on top of the text, however, it's not actually draggable from the text, the text is selectable instead. Would be good to change the mouse pointer.
Preformatted block displays dragging handle, however, it's almost impossible to find a place where to drag it actually, it seems to allow dragging only somewhere near the bottom border.
The resizing handles are a bit inside the block for Text block:

@swissspidy Do you think that any of these are critical?

swissspidy · 2019-06-10T13:55:44Z

@miina Not critical, but also rather easy to fix/improve.

The behavior for the list and preformatted blocks is quite the same. The drag handle is always shown and in my experience always works - unless you're precisely hovering over the text itself (because the browser would start text selection). Here's what they look like in the DOM:

Instead of the ul and pre elements having margins by default, we could add some padding on their parents instead. Plus, they probably need the same height: 100% changes.

The resizing handles are a bit inside the block for Text block:

Will update the CSS there 👍

googlebot · 2019-06-10T14:09:40Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

swissspidy · 2019-06-10T14:12:47Z

@miina Just pushed some commits that already improve the drag handling for these core blocks quite a bit.

miina · 2019-06-10T14:20:11Z

Thanks, @swissspidy, better now! 👍

I'm still seeing this with the list block, is it different for you? Basically it doesn't seem to be draggable from the upper part of the block. Perhaps it's OK for now since it's quite close to the text, however, wondering if it would need similar handling to the Text block 🤷‍♀

swissspidy · 2019-06-10T14:32:54Z

@miina Yup, that's the margin/padding situation I mentioned. Pushed a commit now that improves this a bit for the list block. We can revisit and fine-tune in the future.

miina

Looks good for now!

googlebot · 2019-06-10T15:12:39Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

kienstra added 3 commits May 23, 2019 01:55

Begin the proof-of-concept for draggign the entire block

ed04be2

This has several bugs, and might not be the approach that we go with. But you can now drag the entire block for some blocks, like text and "meta" blocks.

Address ESLint errors causing failed Travis build

1d4104f

There were several errors, so address those here.

Address another ESLint error: useless constructor

f5f27f2

googlebot added the cla: yes Signed the Google CLA label May 23, 2019

kienstra added the AMP-Stories label May 23, 2019

kienstra added 6 commits May 23, 2019 18:37

Merge in develop, resolve conflict

6959b5c

The only conflict was in: with-amp-story-settings.js Retain edits in both branches.

Remove styling that no longer applies

98d3e88

Much of the styling in edit.css applied to the drag handle, which is removed.

Update block-mover/README.md to reflect for the modified files

2d133c9

Some files were modified in the latest commits, so note that here.

kienstra commented May 27, 2019

View reviewed changes

kienstra changed the title ~~[WIP] Proof-of-concept of dragging entire blocks~~ Proof-of-concept of dragging entire blocks May 27, 2019

kienstra requested review from miina and swissspidy May 27, 2019 03:19

kienstra commented May 27, 2019

View reviewed changes

kienstra mentioned this pull request May 27, 2019

Consider removing drag handle in favor of dragging the block itself #2275

Closed

swissspidy reviewed May 27, 2019

View reviewed changes

Merge branch 'develop' into try/drag-entire-block

1832981

kienstra added 2 commits May 27, 2019 10:44

Remove vendor-prefixed properties that don't exist

cae00b7

As Pascal mentioned, it looks like these don't actually exist.

Use cursor: grab for the draggable area of blocks

ef75b10

As suggested on the PR, add this, but change it back to the default for the editable area of text on the Text block

swissspidy added 4 commits June 10, 2019 12:52

Further increase space around border so it is even

75cff31

Fix layout of meta blocks

dea7875

Ensures that inner elements of meta blocks always use the full height. That way one use the full occupied area for dragging and text is propperly vertically centered when using amp-fit-text.

Simplify styles

fbdd766

Tweak numbers

8493773

swissspidy added cla: yes Signed the Google CLA and removed cla: no Has not signed the Google CLA labels Jun 10, 2019

swissspidy approved these changes Jun 10, 2019

View reviewed changes

swissspidy requested a review from miina June 10, 2019 11:22

miina reviewed Jun 10, 2019

View reviewed changes

swissspidy added 2 commits June 10, 2019 16:00

Improve resiz container CSS for text block

9be6087

Improve styles for some core blocks

b520dbb

googlebot added cla: no Has not signed the Google CLA and removed cla: yes Signed the Google CLA labels Jun 10, 2019

List block: swap margin for padding

e76c073

miina approved these changes Jun 10, 2019

View reviewed changes

swissspidy added cla: yes Signed the Google CLA and removed cla: no Has not signed the Google CLA labels Jun 10, 2019

swissspidy changed the title ~~Proof-of-concept of dragging entire blocks~~ Allow dragging entire blocks Jun 10, 2019

swissspidy merged commit ca17450 into develop Jun 10, 2019

swissspidy deleted the try/drag-entire-block branch June 10, 2019 15:14

This was referenced Jun 10, 2019

Resizing text block causes validation error #2556

Closed

Encourage lighter images and videos #2509

Merged

Conversation

kienstra commented May 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kienstra May 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kienstra May 27, 2019

Choose a reason for hiding this comment

Uh oh!

kienstra commented May 27, 2019

Uh oh!

kienstra commented May 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kienstra May 27, 2019

Choose a reason for hiding this comment

Uh oh!

miina commented May 27, 2019

Uh oh!

miina commented May 27, 2019

Uh oh!

swissspidy May 27, 2019

Choose a reason for hiding this comment

Uh oh!

kienstra May 27, 2019

Choose a reason for hiding this comment

Uh oh!

kienstra May 27, 2019

Choose a reason for hiding this comment

Uh oh!

swissspidy commented May 27, 2019

Uh oh!

ThierryA commented May 27, 2019

Uh oh!

kienstra commented May 28, 2019

Uh oh!

kienstra commented May 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

googlebot commented Jun 10, 2019

Uh oh!

swissspidy left a comment

Choose a reason for hiding this comment

Uh oh!

miina Jun 10, 2019

Choose a reason for hiding this comment

Uh oh!

miina commented Jun 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swissspidy commented Jun 10, 2019

Uh oh!

googlebot commented Jun 10, 2019

Uh oh!

swissspidy commented Jun 10, 2019

Uh oh!

miina commented Jun 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swissspidy commented Jun 10, 2019

Uh oh!

miina left a comment

Choose a reason for hiding this comment

Uh oh!

googlebot commented Jun 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kienstra commented May 23, 2019 •

edited

Loading

kienstra May 27, 2019 •

edited

Loading

kienstra commented May 27, 2019 •

edited

Loading

kienstra commented May 28, 2019 •

edited

Loading

miina commented Jun 10, 2019 •

edited

Loading

miina commented Jun 10, 2019 •

edited

Loading