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.

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

[WIP] [AMP Stories] Block Rotation #2014

Merged
merged 40 commits into from
Apr 11, 2019

Conversation

swissspidy
Copy link
Collaborator

@swissspidy swissspidy commented Mar 21, 2019

To Do

  • Ensure angle calculation is correct
  • Fix A11y ESLint issues
    The rotation handle should probably be declared as a switch
  • Fix issue where one can still type in text block even when it is not selected anymore

Screenshots

Screenshot 2019-03-21 at 21 38 46

Screenshot 2019-03-21 at 21 38 28

Screenshot 2019-03-21 at 21 38 24

Fixes #2005

@googlebot googlebot added the cla: yes Signed the Google CLA label Mar 21, 2019
@swissspidy swissspidy marked this pull request as ready for review March 26, 2019 14:17
@swissspidy
Copy link
Collaborator Author

Right now there is a little jump when the CSS transform changes from the one element to the other, making it look like the block rotates back and forth.

Not sure how this could best be prevented.

@swissspidy swissspidy requested review from miina and amedina March 26, 2019 14:19
@miina
Copy link
Contributor

miina commented Mar 26, 2019

Some notes:

  • The grab handle area seems to be a little bit off from the actual grab handle dot, meaning that clicking and trying to grab directly from the dot doesn't seem to work.
  • On rotate start, the element jumps so that it moves away from the pointer. It seems to be depending on the current position how it "jumps", for example in some cases the element rotates 180 degrees so that the mouse pointer for rotating is actually on the opposite side of the element. Did you mean this by the "jumping" in your comment ([WIP] [AMP Stories] Block Rotation #2014 (comment)) or was that another thing?
  • After rotating and then dragging, the element seems to cause setting 0deg in the editor and it's not rotated anymore.
  • Same for resizing -- when resizing a rotated Text block it seems to lose its rotation (in the editor view, selected block). And when trying to rotate again after this then it doesn't seem to have effect in the editor.

Block selected:
Screen Shot 2019-03-26 at 9 19 43 PM

Block not selected:
Screen Shot 2019-03-26 at 9 19 38 PM

@swissspidy
Copy link
Collaborator Author

The grab handle area seems to be a little bit off from the actual grab handle dot, meaning that clicking and trying to grab directly from the dot doesn't seem to work.

I cannot reproduce this. The area is a bit bigger to make grabbing easier, but clicking directly on the dot should work too. Here's the clickable area:

Screenshot 2019-03-28 at 12 45 43

in some cases the element rotates 180 degrees so that the mouse pointer for rotating is actually on the opposite side of the element. Did you mean this by the "jumping" in your comment (#2014 (comment)) or was that another thing?

Sort of.. I cannot reproduce a situation where I am rotating a block and the pointer is on the opposite. Same for your other findings actually. 🤔

Could you perhaps record your screen or something?

My thinking with the rotation here was basically this:

First of all, while rotation information is stored in a block attribute and added as an inline style to the .wp-block element, during rotation the style attribute of the inner .amp-story-editor__rotate-container element will be updated.

Selected Block:

When a block is upside down, one should still be able to edit it. Thus, rotation is ignored for selected blocks (transform: rotate(0deg) !important;)

Now, when you start rotating a selected block, the inner element will be rotated, starting from 0. When you stop, the block's attribute is updated (and thus its inline style).

At this point, you want to show the user that the rotation was successful, so two things happen:

  • Rotation of the inner element gets reset to 0
  • The block gets un-selected and focus is removed

The change of rotation here from the inner element to the block causes some sort of "jump" as you can see in this GIF:

jump-selected

Un-selected Block

Since the block isn't selected, it is shown rotated.

Upon rotation start, block rotation will be ignored and added to the inner element instead. After finish, the block attribute is updated and the inner element's rotation reset. This causes a "jump" as well:

unselected


On another note: Moving around a rotated block does not seem to work. I'll have to investigate that one. Super odd.

@swissspidy
Copy link
Collaborator Author

The issue seems to be that the calculation is based off the block's getBoundingClientRect(), which uses the position after any CSS transforms are applied. Maybe we need to do some maths there to account for this. Or perhaps there are some alternatives 🤷‍♂️

@swissspidy
Copy link
Collaborator Author

@miina Since you worked on the BlockMover, maybe you have an idea here? :-)

@miina
Copy link
Contributor

miina commented Apr 2, 2019

@swissspidy It feels like moving the block would make more sense while it's rotated (not selected) to understand where it would be positioned. Otherwise it could anyway end up in an unexpected location.

At this moment when trying to drag the element, it initially just selects, meaning that it's not possible to drag in a rotated position. I wonder if there's a way to remove the rotation only if ! isDragging?

@miina
Copy link
Contributor

miina commented Apr 2, 2019

Alternatively, maybe the rotation could be applied only to the clone that's being dragged.

@swissspidy
Copy link
Collaborator Author

moving the block would make more sense while it's rotated (not selected) to understand where it would be positioned.

Yep, that's my goal. You can test it by commenting out the relevant CSS for a bit. Passing the rotation angle is also possible. Some WIP code:

diff --git assets/css/amp-editor-story-blocks.css assets/css/amp-editor-story-blocks.css
index 4097d147..df7f7384 100644
--- assets/css/amp-editor-story-blocks.css
+++ assets/css/amp-editor-story-blocks.css
@@ -1048,10 +1048,12 @@ div[data-type="amp/amp-story-page"] .block-editor-inner-blocks .block-editor-blo
 	transition: transform .3s ease-in-out;
 }
 
+/*
 .block-editor-block-list__block.is-typing:not(.is-rotating),
 .block-editor-block-list__block.is-selected:not(.is-rotating) {
 	transform: rotate(0deg) !important;
 }
+*/
 
 .editor-inner-blocks .block-editor-block-list__block.is-rotating .block-editor-block-list__insertion-point,
 .editor-inner-blocks .block-editor-block-list__block.is-rotating .editor-block-mover,
diff --git assets/src/components/block-mover/block-draggable.js assets/src/components/block-mover/block-draggable.js
index d25c0f8b..df41956e 100644
--- assets/src/components/block-mover/block-draggable.js
+++ assets/src/components/block-mover/block-draggable.js
@@ -12,7 +12,7 @@ import { withSelect } from '@wordpress/data';
  */
 import Draggable from './draggable';
 
-const BlockDraggable = ( { children, clientId, rootClientId, blockElementId, index, onDragStart, onDragEnd } ) => {
+const BlockDraggable = ( { children, clientId, rootClientId, blockElementId, rotationAngle, index, onDragStart, onDragEnd } ) => {
 	const transferData = {
 		type: 'block',
 		srcIndex: index,
@@ -23,6 +23,7 @@ const BlockDraggable = ( { children, clientId, rootClientId, blockElementId, ind
 	return (
 		<Draggable
 			elementId={ blockElementId }
+			rotationAngle={ rotationAngle }
 			transferData={ transferData }
 			onDragStart={ onDragStart }
 			onDragEnd={ onDragEnd }
diff --git assets/src/components/block-mover/drag-handle.js assets/src/components/block-mover/drag-handle.js
index f0c5c492..26648bff 100644
--- assets/src/components/block-mover/drag-handle.js
+++ assets/src/components/block-mover/drag-handle.js
@@ -8,7 +8,7 @@ import classnames from 'classnames';
  */
 import BlockDraggable from './block-draggable';
 
-export const IconDragHandle = ( { isVisible, className, icon, onDragStart, onDragEnd, blockElementId, clientId } ) => {
+export const IconDragHandle = ( { isVisible, className, icon, onDragStart, onDragEnd, blockElementId, rotationAngle, clientId } ) => {
 	if ( ! isVisible ) {
 		return null;
 	}
@@ -19,6 +19,7 @@ export const IconDragHandle = ( { isVisible, className, icon, onDragStart, onDra
 		<BlockDraggable
 			clientId={ clientId }
 			blockElementId={ blockElementId }
+			rotationAngle={ rotationAngle }
 			onDragStart={ onDragStart }
 			onDragEnd={ onDragEnd }
 		>
diff --git assets/src/components/block-mover/draggable.js assets/src/components/block-mover/draggable.js
index 3b3d6f92..7dd2a802 100644
--- assets/src/components/block-mover/draggable.js
+++ assets/src/components/block-mover/draggable.js
@@ -75,12 +75,14 @@ class Draggable extends Component {
 	 *  - Clones the current element and spawns clone over original element.
 	 *  - Sets transfer data.
 	 *  - Adds dragover listener.
-	 * @param  {Object} event					The non-custom DragEvent.
-	 * @param  {string} elementId				The HTML id of the element to be dragged.
-	 * @param  {Object} transferData			The data to be set to the event's dataTransfer - to be accessible in any later drop logic.
+	 *
+	 * @param  {Object} event		  The non-custom DragEvent.
+	 * @param  {string} elementId	  The HTML id of the element to be dragged.
+	 * @param  {number} rotationAngle The HTML id of the element to be dragged.
+	 * @param  {Object} transferData  The data to be set to the event's dataTransfer - to be accessible in any later drop logic.
 	 */
 	onDragStart( event ) {
-		const { elementId, transferData, onDragStart = noop } = this.props;
+		const { elementId, rotationAngle, transferData, onDragStart = noop } = this.props;
 		const element = document.getElementById( elementId );
 		const parentPage = element.closest( 'div[data-type="amp/amp-story-page"]' );
 		if ( ! element || ! parentPage ) {
@@ -93,25 +95,27 @@ class Draggable extends Component {
 		// Prepare element clone and append to element wrapper.
 		const elementRect = element.getBoundingClientRect();
 		const elementWrapper = element.parentNode;
-		const elementTopOffset = parseInt( elementRect.top, 10 );
-		const elementLeftOffset = parseInt( elementRect.left, 10 );
 
 		const pageWrapperRect = parentPage.getBoundingClientRect();
 
 		const clone = element.cloneNode( true );
+		const elementTopOffset = parseInt( elementRect.top, 10 );
+		const elementLeftOffset = parseInt( elementRect.left, 10 );
 		clone.id = `clone-${ elementId }`;
 		clone.style.top = 0;
 		clone.style.left = 0;
+		clone.style.transform = null;
 		this.cloneWrapper = document.createElement( 'div' );
 		this.cloneWrapper.classList.add( cloneWrapperClass );
 		this.cloneWrapper.style.width = `${ elementRect.width }px`;
+		this.cloneWrapper.style.transform = `rotate(${ rotationAngle || 0 }deg)`;
+
 
 		// Position clone over the original element.
 		this.cloneWrapper.style.top = `${ elementTopOffset - parseInt( pageWrapperRect.top, 10 ) }px`;
 		// Add 5px adjustment for having the block mover right next to the clone.
 		// @todo This will need some additional adjusting once we add padding to the Page in the editor.
 		this.cloneWrapper.style.left = `${ elementLeftOffset - parseInt( pageWrapperRect.left, 10 ) - 5 }px`;
-
 		// Hack: Remove iFrames as it's causing the embeds drag clone to freeze
 		[ ...clone.querySelectorAll( 'iframe' ) ].forEach( ( child ) => child.parentNode.removeChild( child ) );
 
diff --git assets/src/components/block-mover/index.js assets/src/components/block-mover/index.js
index 93e0baa1..8bfcdf62 100644
--- assets/src/components/block-mover/index.js
+++ assets/src/components/block-mover/index.js
@@ -49,7 +49,19 @@ export class BlockMover extends Component {
 	}
 
 	render() {
-		const { onMoveUp, onMoveDown, isFirst, isLast, isDraggable, onDragStart, onDragEnd, clientIds, blockElementId, instanceId } = this.props;
+		const {
+			onMoveUp,
+			onMoveDown,
+			isFirst,
+			isLast,
+			isDraggable,
+			onDragStart,
+			onDragEnd,
+			clientIds,
+			blockElementId,
+			rotationAngle,
+			instanceId,
+		} = this.props;
 		const { isFocused } = this.state;
 
 		// We emulate a disabled state because forcefully applying the `disabled`
@@ -74,6 +86,7 @@ export class BlockMover extends Component {
 						icon={ dragHandle }
 						clientId={ clientIds }
 						blockElementId={ blockElementId }
+						rotationAngle={ rotationAngle }
 						isVisible={ isDraggable }
 						onDragStart={ onDragStart }
 						onDragEnd={ onDragEnd }
diff --git assets/src/components/with-amp-story-settings.js assets/src/components/with-amp-story-settings.js
index 86a56a5b..0c20c132 100644
--- assets/src/components/with-amp-story-settings.js
+++ assets/src/components/with-amp-story-settings.js
@@ -22,7 +22,7 @@ export default createHigherOrderComponent(
 				return <BlockEdit { ...props } />;
 			}
 
-			const { ampShowImageCaption } = attributes;
+			const { ampShowImageCaption, rotationAngle } = attributes;
 			const isImageBlock = 'core/image' === name;
 
 			return (
@@ -30,6 +30,7 @@ export default createHigherOrderComponent(
 					<StoryBlockMover
 						clientIds={ props.clientId }
 						blockElementId={ `block-${ props.clientId }` }
+						rotationAngle={ rotationAngle }
 						isFirst={ props.isFirst }
 						isLast={ props.isLast }
 						isFocused={ props.isFocused }

@miina
Copy link
Contributor

miina commented Apr 8, 2019

@swissspidy To clarify: did you already try using the rotation on cloneWrapper via this.cloneWrapper.style.transform =rotate(${ rotationAngle || 0 }deg); and the issue was what you mentioned within #2014 (comment)?

@swissspidy
Copy link
Collaborator Author

@miina Yes, exactly. I'm gonna give it another go.

@swissspidy
Copy link
Collaborator Author

While working this I noticed that

a) the default block mover was still being displayed, but just overlaid by our custom one
b) our custom block mover did not disable the up/down arrows when a block is first/last in list

This has now been fixed. That means one cannot send a block forward when it is already at the top of the list, or send it backward when it is already at the end.

Next, accurately calculating the rotated clone's position in relation to the page, so that the actual block can be positioned accordingly, turned out to be a bit too complex to get it right in time.

Hence, I decided to make this simpler for now:

  • no block mover visible when a block is not selected
  • only selected blocks can be drag & dropped at the moment
  • while selecting & dragging, the block is not rotated
  • after dragging is finished, the block will be rotated again

Not selected block:

Screenshot 2019-04-10 at 15 24 19

Hovering:

Screenshot 2019-04-10 at 15 13 57
Screenshot 2019-04-10 at 15 13 50

Selected / Rotating:

Screenshot 2019-04-10 at 15 14 03

Dragging:

Screenshot 2019-04-10 at 15 14 09 (3)

Demo:

rotation (1)

@swissspidy swissspidy requested review from miina and amedina April 10, 2019 13:42
assets/src/components/rotatable-box/index.js Outdated Show resolved Hide resolved
assets/src/components/rotatable-box/index.js Outdated Show resolved Hide resolved
assets/src/stores/amp-story/actions.js Outdated Show resolved Hide resolved
@miina
Copy link
Contributor

miina commented Apr 10, 2019

Tested it and seems to works as expected. Would be ideal if we could drag a rotated block, too, but maybe that's something we'll be able to add later in case it should become necessary to position something very precisely.

the default block mover was still being displayed, but just overlaid by our custom one

Good catch! The default block mover classnames were updated at some point, however, couldn't find it hidden with the old class name either (thought that perhaps the old one is still there), not sure where it disappeared.

@swissspidy
Copy link
Collaborator Author

Would be ideal if we could drag a rotated block, too, but maybe that's something we'll be able to add later in case it should become necessary to position something very precisely.

That's my goal too, yes.

@swissspidy
Copy link
Collaborator Author

Alright, addressed the feedback in the last few commits.

I'm gonna merge this for now. It works well enough for alpha, but we definitely need to revisit the dragging part after that. I created a card for it in the backlog already.

Of course some minor tweaks could already happen before that.

@swissspidy swissspidy merged commit 9b7416d into amp-stories-redux Apr 11, 2019
@swissspidy swissspidy deleted the amp-story/2005-rotation branch April 11, 2019 15:38
@westonruter westonruter added this to the v1.2 milestone May 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Signed the Google CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants