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

Allow drag to different page #3312

Merged
merged 37 commits into from
Oct 4, 2019
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
6e37b45
Allow drag to different page
barklund Sep 20, 2019
ba20251
Refactor duplicated logic
barklund Sep 20, 2019
354443b
Remove unnecessary hook
barklund Sep 20, 2019
40259f8
More performant structure
barklund Sep 23, 2019
9fd43c3
Better separation of concerns
barklund Sep 23, 2019
305b692
Handle CTA blocks correctly when dragging between pages
barklund Sep 23, 2019
f7d0c19
Fix proptypes and add inline comment
barklund Sep 23, 2019
83b2e4f
Fix offset bug when dragging 2+ pages left
barklund Sep 23, 2019
3928f60
Adding documentation for functions
barklund Sep 24, 2019
efd22fd
Fix disallow duplicate CTA
barklund Sep 24, 2019
95ca6c8
Generalised helper for checking if block is allowed on given page
barklund Sep 26, 2019
6109bec
Refactored `isCTABlock` to helpers
barklund Sep 26, 2019
b3be9f5
Rename variable and move to constants
barklund Sep 27, 2019
0395f30
Merge branch 'develop' into feature/3311-allow-drag-to-page
barklund Sep 27, 2019
04fffea
Make props required where applicable
barklund Sep 27, 2019
3c7afcf
Reorganised helper to aid in testing
barklund Sep 27, 2019
aee7550
Fix typos
barklund Sep 27, 2019
64f341d
Reuse `STORY_PAGE_MARGIN` where applicable
barklund Sep 27, 2019
5160c2f
Missing rewrite to `isCTABlock` fixed
barklund Sep 27, 2019
9bcb884
Fix minor errors in rewrites
barklund Sep 27, 2019
7ee41f3
Add `aria-grabbed` to the block mover
barklund Sep 27, 2019
3b8a139
Revert "Add `aria-grabbed` to the block mover"
barklund Sep 27, 2019
4f4f619
Fix test wording
barklund Sep 30, 2019
d1813e1
Updated component documentation
barklund Oct 1, 2019
c3e51f4
Updated logic to display non-allowed drop zones
barklund Oct 1, 2019
4041030
Add border on allowed regions instead (and fix css lint)
barklund Oct 1, 2019
1cde15c
Major rewrite of entire implementation
barklund Oct 2, 2019
3f13272
Merge branch 'develop' into feature/3311-allow-drag-to-page
barklund Oct 3, 2019
e963844
Convert helper function to hook
barklund Oct 3, 2019
3241e3b
Added dependencies for `useSelect`
barklund Oct 3, 2019
005bac0
Created new constant for the height of the drop zone for CTAs
barklund Oct 3, 2019
354ee52
Updated `useMoveBlockToPage` and added tests
barklund Oct 3, 2019
8f068be
Fix proptype missed in refactor
barklund Oct 3, 2019
d5eb6a8
Merge branch 'develop' into feature/3311-allow-drag-to-page
swissspidy Oct 4, 2019
144c430
Refactor MediaInserter to use hooks
swissspidy Oct 4, 2019
60f0198
Move isBlockAllowedOnPage function inline
swissspidy Oct 4, 2019
9bd9ab3
Remove ifCondition HOC usage from stories editor
swissspidy Oct 4, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions assets/src/stories-editor/blocks/amp-story-page/edit.css
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,10 @@
opacity: .5;
}

.editor-block-list__layout [data-type="amp/amp-story-page"].amp-page-draggable-hover {
opacity: 1;
}

.editor-block-list__layout [data-type="amp/amp-story-page"].amp-page-inactive > div {
pointer-events: none;
}
Expand Down
122 changes: 112 additions & 10 deletions assets/src/stories-editor/components/block-mover/block-draggable.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,28 +10,64 @@ import PropTypes from 'prop-types';
/**
* WordPress dependencies
*/
import { withSelect } from '@wordpress/data';
import { withDispatch, withSelect } from '@wordpress/data';
import { compose } from '@wordpress/compose';
import { cloneBlock } from '@wordpress/blocks';

/**
* Internal dependencies
*/
import Draggable from './draggable';
import { isBlockAllowedOnPage } from '../../helpers';

const BlockDraggable = ( { children, clientId, blockName, rootClientId, blockElementId, index, onDragStart, onDragEnd } ) => {
const BlockDraggable = ( { children, clientId, blockName, rootClientId, blockElementId, index, onDragStart, onDragEnd, onNeighborDrop, getNeighborPageId } ) => {
const transferData = {
type: 'block',
srcIndex: index,
srcRootClientId: rootClientId,
srcClientId: clientId,
};

// This holds the currently highlighted element
const currentHoverElement = { pageId: null, current: null };

/**
* Highlight neighboring page on hover. Neigboring page is given by offset.
*
* @param {number} offset Integer specifying offset from current page - e.g. -2 on page 3 will return id of page 1. Page indices are zero-based.
*/
const onNeighborHover = ( offset ) => {
// Get neighboring page.
const newPageId = getNeighborPageId( offset );
const hasHighlightChanged = newPageId !== currentHoverElement.pageId;

if ( ! hasHighlightChanged ) {
return;
}

currentHoverElement.pageId = newPageId;

// Unhighlight old highlighted page.
if ( currentHoverElement.current ) {
currentHoverElement.current.classList.remove( 'amp-page-draggable-hover' );
}

// Highlight neigboring page.
if ( newPageId ) {
currentHoverElement.current = document.getElementById( `block-${ newPageId }` );
currentHoverElement.current.classList.add( 'amp-page-draggable-hover' );
}
};

return (
<Draggable
blockName={ blockName }
elementId={ blockElementId }
transferData={ transferData }
onDragStart={ onDragStart }
onDragEnd={ onDragEnd }
onNeighborDrop={ onNeighborDrop }
onNeighborHover={ onNeighborHover }
>
{
( { onDraggableStart, onDraggableEnd } ) => {
Expand All @@ -54,13 +90,79 @@ BlockDraggable.propTypes = {
children: PropTypes.any.isRequired,
onDragStart: PropTypes.func,
onDragEnd: PropTypes.func,
onNeighborHover: PropTypes.func,
onNeighborDrop: PropTypes.func,
getNeighborPageId: PropTypes.func,
spacedmonkey marked this conversation as resolved.
Show resolved Hide resolved
};

export default withSelect( ( select, { clientId } ) => {
const { getBlockIndex, getBlockRootClientId } = select( 'core/block-editor' );
const rootClientId = getBlockRootClientId( clientId );
return {
index: getBlockIndex( clientId, rootClientId ),
rootClientId,
};
} )( BlockDraggable );
export default compose(
withSelect( ( select, { clientId } ) => {
const { getBlockIndex, getBlockRootClientId, getBlockOrder, getBlock } = select( 'core/block-editor' );
const rootClientId = getBlockRootClientId( clientId );
return {
index: getBlockIndex( clientId, rootClientId ),
rootClientId,
block: getBlock( clientId ),
getBlockOrder,
};
} ),
withDispatch( ( dispatch, { getBlockOrder, rootClientId, clientId, block } ) => {
const { setCurrentPage } = dispatch( 'amp/story' );
const { selectBlock, removeBlock, insertBlock, updateBlockAttributes } = dispatch( 'core/block-editor' );

barklund marked this conversation as resolved.
Show resolved Hide resolved
/**
* Get id of neighbor page that is `offset` away from the current page.
*
* If no page exists in that direction, if offset is zero or if element is not allowed on that page, null will be returned.
*
* @param {number} offset Integer specifying offset from current page - e.g. -2 on page 3 will return id of page 1. Page indices are zero-based.
* @return {string} Returns id of target page or null if element can't be dropped there for any reason.
*/
const getNeighborPageId = ( offset ) => {
barklund marked this conversation as resolved.
Show resolved Hide resolved
const pages = getBlockOrder();
const currentPageIndex = pages.findIndex( ( i ) => i === rootClientId );
const newPageIndex = currentPageIndex + offset;
const isInsidePageCount = newPageIndex >= 0 && newPageIndex < pages.length;
const newPageId = pages[ newPageIndex ];
const isAllowedOnPage = isBlockAllowedOnPage( block.name, newPageId );

// Do we even have a legal neighbor in that direction? (offset=0 is not a neighbor)
if ( offset === 0 || ! isInsidePageCount || ! isAllowedOnPage ) {
return null;
}

return newPageId;
};

/**
* Drop this element on the page `offset` away from the current page (if possible). If dropped, update the position of the element on the new page.
*
* Currently this function removes the old element and creates a clone on the new page.
*
* @param {number} offset Integer specifying offset from current page - e.g. -2 on page 3 will return id of page 1. Page indices are zero-based.
* @param {Object} newAttributes Object with attributes to update on element on new page.
*/
const onNeighborDrop = ( offset, newAttributes ) => {
barklund marked this conversation as resolved.
Show resolved Hide resolved
const newPageId = getNeighborPageId( offset );
if ( ! newPageId ) {
return;
}

// Remove block and add cloned block to new page.
removeBlock( clientId );
barklund marked this conversation as resolved.
Show resolved Hide resolved
const clonedBlock = cloneBlock( block );
insertBlock( clonedBlock, null, newPageId );
updateBlockAttributes( clonedBlock.clientId, newAttributes );

// Switch to new page.
setCurrentPage( newPageId );
barklund marked this conversation as resolved.
Show resolved Hide resolved
selectBlock( newPageId );
};

return {
onNeighborDrop,
getNeighborPageId,
};
} ),
)( BlockDraggable );

58 changes: 53 additions & 5 deletions assets/src/stories-editor/components/block-mover/draggable.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,15 @@ import { withSafeTimeout } from '@wordpress/compose';
/**
* Internal dependencies
*/
import { getPixelsFromPercentage } from '../../helpers';
import { getPixelsFromPercentage, getPercentageFromPixels, getRelativeElementPosition } from '../../helpers';
import {
STORY_PAGE_INNER_WIDTH,
STORY_PAGE_INNER_HEIGHT,
} from '../../constants';

const PAGE_BORDER = 50;
barklund marked this conversation as resolved.
Show resolved Hide resolved
const PAGE_AND_BORDER = STORY_PAGE_INNER_WIDTH + PAGE_BORDER;

const { Image, navigator } = window;

const cloneWrapperClass = 'components-draggable__clone';
Expand Down Expand Up @@ -61,13 +64,33 @@ class Draggable extends Component {
* @param {Object} event The non-custom DragEvent.
*/
onDragEnd = ( event ) => {
const { onDragEnd = noop } = this.props;
const { onNeighborDrop, blockName, setTimeout, onDragEnd = noop } = this.props;
if ( event ) {
event.preventDefault();
}

// Attempt drop on neighbor if offset
if ( this.pageOffset !== 0 ) {
// All this is about calculating the position of the (correct) element on the new page.
const currentElementTop = parseInt( this.cloneWrapper.style.top );
const currentElementLeft = parseInt( this.cloneWrapper.style.left );
const newLeft = currentElementLeft - ( this.pageOffset * PAGE_AND_BORDER );
const isCTABlock = 'amp/amp-story-cta' === blockName;
barklund marked this conversation as resolved.
Show resolved Hide resolved
const baseHeight = isCTABlock ? STORY_PAGE_INNER_HEIGHT / 5 : STORY_PAGE_INNER_HEIGHT;
spacedmonkey marked this conversation as resolved.
Show resolved Hide resolved
const x = getPercentageFromPixels( 'x', newLeft, STORY_PAGE_INNER_WIDTH );
const y = getPercentageFromPixels( 'y', currentElementTop, baseHeight );
const xAttribute = isCTABlock ? 'btnPositionLeft' : 'positionLeft';
const yAttribute = isCTABlock ? 'btnPositionTop' : 'positionTop';
const newAttributes = {
[ xAttribute ]: x,
[ yAttribute ]: y,
};

onNeighborDrop( this.pageOffset, newAttributes );
}

this.resetDragState();
this.props.setTimeout( onDragEnd );
setTimeout( onDragEnd );
}

/**
Expand All @@ -76,6 +99,7 @@ class Draggable extends Component {
* @param {Object} event The non-custom DragEvent.
*/
onDragOver = ( event ) => {
const { onNeighborHover } = this.props;
const top = parseInt( this.cloneWrapper.style.top ) + event.clientY - this.cursorTop;

// Don't allow the CTA button to go over its top limit.
Expand All @@ -91,6 +115,21 @@ class Draggable extends Component {
// Update cursor coordinates.
this.cursorLeft = event.clientX;
this.cursorTop = event.clientY;

// Check if mouse (*not* element, but actual cursor) is over neighboring page to either side.
const currentElementLeft = parseInt( this.cloneWrapper.style.left );
const cursorLeftRelativeToPage = currentElementLeft + this.cursorLeftInsideElement;
const isOffRight = cursorLeftRelativeToPage > PAGE_AND_BORDER;
const isOffLeft = cursorLeftRelativeToPage < -PAGE_BORDER;
this.pageOffset = 0;
if ( isOffLeft || isOffRight ) {
// Check how far off we are to that side - on large screens you can drag elements 2+ pages over to either side.
this.pageOffset = ( isOffLeft ?
-Math.ceil( ( -PAGE_BORDER - cursorLeftRelativeToPage ) / PAGE_AND_BORDER ) :
Math.ceil( ( cursorLeftRelativeToPage - PAGE_AND_BORDER ) / PAGE_AND_BORDER )
);
}
onNeighborHover( this.pageOffset );
}

onDrop = () => {
Expand Down Expand Up @@ -152,8 +191,15 @@ class Draggable extends Component {
const baseHeight = isCTABlock ? STORY_PAGE_INNER_HEIGHT / 5 : STORY_PAGE_INNER_HEIGHT;

// Position clone over the original element.
this.cloneWrapper.style.top = `${ getPixelsFromPercentage( 'y', parseInt( clone.style.top ), baseHeight ) }px`;
this.cloneWrapper.style.left = `${ getPixelsFromPercentage( 'x', parseInt( clone.style.left ), STORY_PAGE_INNER_WIDTH ) }px`;
const top = getPixelsFromPercentage( 'y', parseInt( clone.style.top ), baseHeight );
const left = getPixelsFromPercentage( 'x', parseInt( clone.style.left ), STORY_PAGE_INNER_WIDTH );
this.cloneWrapper.style.top = `${ top }px`;
this.cloneWrapper.style.left = `${ left }px`;

// Get starting position information
const absolutePositionOfPage = getRelativeElementPosition( elementWrapper, document.documentElement );
const absoluteElementLeft = absolutePositionOfPage.left + left;
this.cursorLeftInsideElement = event.clientX - absoluteElementLeft;

clone.id = `clone-${ elementId }`;
clone.style.top = 0;
Expand Down Expand Up @@ -224,6 +270,8 @@ Draggable.propTypes = {
transferData: PropTypes.object,
onDragStart: PropTypes.func,
onDragEnd: PropTypes.func,
onNeighborDrop: PropTypes.func,
onNeighborHover: PropTypes.func,
setTimeout: PropTypes.func.isRequired,
children: PropTypes.any.isRequired,
};
Expand Down
7 changes: 2 additions & 5 deletions assets/src/stories-editor/components/inserter/menu.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't the hook be used here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That could work yeah 👍


const MAX_SUGGESTED_ITEMS = 9;

Expand Down Expand Up @@ -377,8 +378,6 @@ export default compose(
getBlockName,
getBlockRootClientId,
getBlockSelectionEnd,
canInsertBlockType,
getBlockListSettings,
} = select( 'core/block-editor' );
const {
getChildBlockNames,
Expand Down Expand Up @@ -409,9 +408,7 @@ export default compose(
return true;
}

// canInsertBlockType() alone is not enough, see https://github.com/WordPress/gutenberg/issues/14515
const destinationBlockListSettings = getBlockListSettings( destinationRootClientId );
return canInsertBlockType( name, getCurrentPage() ) && destinationBlockListSettings && destinationBlockListSettings.allowedBlocks.includes( name );
swissspidy marked this conversation as resolved.
Show resolved Hide resolved
return isBlockAllowedOnPage( name, getCurrentPage() );
} );

return {
Expand Down
11 changes: 4 additions & 7 deletions assets/src/stories-editor/components/media-inserter/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,11 @@ import { withDispatch, withSelect } from '@wordpress/data';
import { DropdownMenu } from '@wordpress/components';
import { compose, ifCondition } from '@wordpress/compose';
import { __, sprintf } from '@wordpress/i18n';

/**
* Internal dependencies
*/
import { processMedia } from '../../helpers';
import { processMedia, isBlockAllowedOnPage } from '../../helpers';
swissspidy marked this conversation as resolved.
Show resolved Hide resolved

const POPOVER_PROPS = {
position: 'bottom right',
Expand Down Expand Up @@ -110,7 +111,7 @@ const mediaPicker = ( dialogTitle, mediaType, updateBlock ) => {

const applyWithSelect = withSelect( ( select ) => {
const { getCurrentPage } = select( 'amp/story' );
const { canInsertBlockType, getBlockListSettings, getBlock } = select( 'core/block-editor' );
const { getBlock } = select( 'core/block-editor' );
const { isReordering } = select( 'amp/story' );

const currentPage = getCurrentPage();
Expand All @@ -119,11 +120,7 @@ const applyWithSelect = withSelect( ( select ) => {

return {
isReordering: isReordering(),
canInsertBlockType: ( name ) => {
// canInsertBlockType() alone is not enough, see https://github.com/WordPress/gutenberg/issues/14515
const blockSettings = getBlockListSettings( currentPage );
return canInsertBlockType( name, currentPage ) && blockSettings && blockSettings.allowedBlocks.includes( name );
},
canInsertBlockType: ( name ) => isBlockAllowedOnPage( name, currentPage ),
// As used in <HeaderToolbar> component
showInserter: select( 'core/edit-post' ).getEditorMode() === 'visual' && select( 'core/editor' ).getEditorSettings().richEditingEnabled,
mediaType,
Expand Down
14 changes: 8 additions & 6 deletions assets/src/stories-editor/components/shortcuts/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@ import { withDispatch, withSelect } from '@wordpress/data';
import { IconButton } from '@wordpress/components';
import { compose, ifCondition } from '@wordpress/compose';

/**
* Internal dependencies
*/
import { isBlockAllowedOnPage } from '../../helpers';

const Shortcuts = ( { insertBlock, canInsertBlockType, showInserter } ) => {
const blocks = [
'amp/amp-story-text',
Expand Down Expand Up @@ -48,16 +53,13 @@ Shortcuts.propTypes = {

const applyWithSelect = withSelect( ( select ) => {
const { getCurrentPage } = select( 'amp/story' );
const { canInsertBlockType, getBlockListSettings } = select( 'core/block-editor' );
const { isReordering } = select( 'amp/story' );

const currentPage = getCurrentPage();

return {
isReordering: isReordering(),
canInsertBlockType: ( name ) => {
// canInsertBlockType() alone is not enough, see https://github.com/WordPress/gutenberg/issues/14515
const blockSettings = getBlockListSettings( getCurrentPage() );
return canInsertBlockType( name, getCurrentPage() ) && blockSettings && blockSettings.allowedBlocks.includes( name );
},
canInsertBlockType: ( name ) => isBlockAllowedOnPage( name, currentPage ),
// As used in <HeaderToolbar> component
showInserter: select( 'core/edit-post' ).getEditorMode() === 'visual' && select( 'core/editor' ).getEditorSettings().richEditingEnabled,
};
Expand Down
Loading