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

Fix/1390 Make "title" a block #2714

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions blocks/library/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,4 @@ import './text-columns';
import './verse';
import './video';
import './audio';
import './post-title';
38 changes: 38 additions & 0 deletions blocks/library/post-title/editor.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
.editor-post-permalink {
display: inline-flex;
align-items: center;
position: absolute;
top: -34px;
box-shadow: $shadow-popover;
border: 1px solid $light-gray-500;
background: $white;
padding: 5px;
font-family: $default-font;
font-size: $default-font-size;
left: 0;
right: 0;

@include break-small() {
left: $block-padding + $block-mover-padding-visible - 2px;
right: $block-padding + $block-mover-padding-visible - 2px;
}
}

.editor-post-permalink__label {
margin: 0 10px;
}

.editor-post-permalink__link {
color: $dark-gray-200;
text-decoration: underline;
margin-right: 10px;
width: 100%;
overflow: hidden;
position: relative;
white-space: nowrap;

&:after {
@include long-content-fade( $size: 20% );
}
}

42 changes: 42 additions & 0 deletions blocks/library/post-title/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/**
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';

/**
* Internal dependencies
*/
import { registerBlockType, source } from '../../api';
import PostTitleEdit from './post-title-edit';

registerBlockType( 'core/post-title', {
title: __( 'Post Title' ),

keywords: [ __( 'title' ) ],
Copy link
Member

Choose a reason for hiding this comment

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

This would be redundant, given that searching "title" would also match the "Post Title" title property, unless it was localized significantly different between "title" and "Post Title".


className: false,

category: 'common',

attributes: {
content: {
type: 'string',
source: source.text(),
},
},

isFixed: true,

edit( { attributes, setAttributes, focus, setFocus } ) {
return <PostTitleEdit
title={ attributes.content }
focus={ focus }
setFocus={ setFocus }
onChange={ value => setAttributes( { content: value } ) }
/>;
},

save( { attributes } ) {
return <h1> { attributes.content } </h1>;
Copy link
Member

Choose a reason for hiding this comment

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

  • Even if we treat title as a block, I would expect its value should be saved as the post_title field of a post, not as an h1 within content
  • I don't think we need the space padding within the <h1>

},
} );
74 changes: 74 additions & 0 deletions blocks/library/post-title/post-permalink.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
/**
Copy link
Member

Choose a reason for hiding this comment

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

editor/post-permalink is redundant with this file and should be removed?

* External dependencies
*/
import { connect } from 'react-redux';

/**
* WordPress dependencies
*/
import { Component } from '@wordpress/element';
import { __ } from '@wordpress/i18n';
import { Dashicon, ClipboardButton, Button } from '@wordpress/components';

/**
* Internal Dependencies
*/
import './editor.scss';
import { isEditedPostNew, getEditedPostAttribute } from '../../../editor/selectors';
Copy link
Member

Choose a reason for hiding this comment

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

We don't want to be reaching out into the editor top-level directory. This could tie into #2473


class PostPermalink extends Component {
constructor() {
super( ...arguments );
this.state = {
showCopyConfirmation: false,
};
this.onCopy = this.onCopy.bind( this );
}

componentWillUnmout() {
clearTimeout( this.dismissCopyConfirmation );
}

onCopy() {
this.setState( {
showCopyConfirmation: true,
} );

clearTimeout( this.dismissCopyConfirmation );
this.dismissCopyConfirmation = setTimeout( () => {
this.setState( {
showCopyConfirmation: false,
} );
}, 4000 );
}

render() {
const { isNew, link } = this.props;
if ( isNew || ! link ) {
return null;
}

return (
<div className="editor-post-permalink">
<Dashicon icon="admin-links" />
<span className="editor-post-permalink__label">{ __( 'Permalink:' ) }</span>
<Button className="editor-post-permalink__link" href={ link } target="_blank">
{ link }
</Button>
<ClipboardButton className="button" text={ link } onCopy={ this.onCopy }>
{ this.state.showCopyConfirmation ? __( 'Copied!' ) : __( 'Copy' ) }
</ClipboardButton>
</div>
);
}
}

export default connect(
( state ) => {
return {
isNew: isEditedPostNew( state ),
link: getEditedPostAttribute( state, 'link' ),
};
}
)( PostPermalink );

24 changes: 24 additions & 0 deletions blocks/library/post-title/post-title-edit.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
import { connect } from 'react-redux';

/**
* WordPress dependencies
*/
import Editable from '../../editable';
import PostPermaLink from './post-permalink';
import { editPost } from '../../../editor/actions';

const PostTitleEdit = ( { title, focus, setFocus, onChange } ) => [
focus && <PostPermaLink />,
<Editable
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this needs to be editable, just a plain input (we don't need to / cannot support rich text).

key="editable"
tagName="h1"
value={ [ title ] }
focus={ focus }
onFocus={ setFocus }
onChange={ onChange }
style={ { textAlign: 'left' } }
formattingControls={ [] } /> ];

export default connect( null, dispatch => ( { onChange: title => {
Copy link
Member

Choose a reason for hiding this comment

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

This line is a bit difficult to understand given the many levels of function nesting and differing implicit return / explicit arrow function blocks it creates.

dispatch( editPost( { title: title[ 0 ] } ) );
Copy link
Member

Choose a reason for hiding this comment

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

What is the value of title here? If we were to consider PostTitleEdit the "presentational" component, I would think it's onChange callback should be called with the value we'd expect to be using, without any further manipulations like we're doing here.

} } ) )( PostTitleEdit );
24 changes: 12 additions & 12 deletions editor/block-mover/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,10 @@ import { getBlockType } from '@wordpress/blocks';
* Internal dependencies
*/
import './style.scss';
import { isFirstBlock, isLastBlock, getBlockIndex, getBlock } from '../selectors';
import { canMoveBlockUp, isLastBlock, getBlockIndex, getBlock } from '../selectors';
import { getBlockMoverLabel } from './mover-label';

function BlockMover( { onMoveUp, onMoveDown, isFirst, isLast, uids, blockType, firstIndex } ) {
function BlockMover( { onMoveUp, onMoveDown, canMoveUp, canMoveDown, uids, blockType, firstIndex } ) {
// We emulate a disabled state because forcefully applying the `disabled`
// attribute on the button while it has focus causes the screen to change
// to an unfocused state (body as active element) without firing blur on,
Expand All @@ -27,42 +27,42 @@ function BlockMover( { onMoveUp, onMoveDown, isFirst, isLast, uids, blockType, f
<div className="editor-block-mover">
<IconButton
className="editor-block-mover__control"
onClick={ isFirst ? null : onMoveUp }
onClick={ ! canMoveUp ? null : onMoveUp }
icon="arrow-up-alt2"
tooltip={ __( 'Move Up' ) }
label={ getBlockMoverLabel(
uids.length,
blockType && blockType.title,
firstIndex,
isFirst,
isLast,
canMoveUp,
canMoveDown,
-1,
) }
aria-disabled={ isFirst }
aria-disabled={ ! canMoveUp }
/>
<IconButton
className="editor-block-mover__control"
onClick={ isLast ? null : onMoveDown }
onClick={ ! canMoveDown ? null : onMoveDown }
icon="arrow-down-alt2"
tooltip={ __( 'Move Down' ) }
label={ getBlockMoverLabel(
uids.length,
blockType && blockType.title,
firstIndex,
isFirst,
isLast,
canMoveUp,
canMoveDown,
1,
) }
aria-disabled={ isLast }
aria-disabled={ ! canMoveDown }
/>
</div>
);
}

export default connect(
( state, ownProps ) => ( {
isFirst: isFirstBlock( state, first( ownProps.uids ) ),
isLast: isLastBlock( state, last( ownProps.uids ) ),
canMoveUp: canMoveBlockUp( state, first( ownProps.uids ) ),
canMoveDown: ! isLastBlock( state, last( ownProps.uids ) ),
firstIndex: getBlockIndex( state, first( ownProps.uids ) ),
blockType: getBlockType( getBlock( state, first( ownProps.uids ) ).name ),
} ),
Expand Down
36 changes: 18 additions & 18 deletions editor/block-mover/mover-label.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,25 +10,25 @@ import { __, sprintf } from '@wordpress/i18n';
* @param {string} type Block type - in the case of a single block, should
* define its 'type'. I.e. 'Text', 'Heading', 'Image' etc.
* @param {number} firstIndex The index (position - 1) of the first block selected.
* @param {boolean} isFirst This is the first block.
* @param {boolean} isLast This is the last block.
* @param {boolean} canMoveUp Indicates whether the first selected block can move up.
* @param {boolean} canMoveDown Indicates whether the last selected block can move down.
* @param {number} dir Direction of movement (> 0 is considered to be going
* down, < 0 is up).
* @return {string} Label for the block movement controls.
*/
export function getBlockMoverLabel( selectedCount, type, firstIndex, isFirst, isLast, dir ) {
export function getBlockMoverLabel( selectedCount, type, firstIndex, canMoveUp, canMoveDown, dir ) {
const position = ( firstIndex + 1 );

if ( selectedCount > 1 ) {
return getMultiBlockMoverLabel( selectedCount, firstIndex, isFirst, isLast, dir );
return getMultiBlockMoverLabel( selectedCount, firstIndex, canMoveUp, canMoveDown, dir );
}

if ( isFirst && isLast ) {
if ( ( ! canMoveUp ) && ( ! canMoveDown ) ) {
// translators: %s: Type of block (i.e. Text, Image etc)
return sprintf( __( 'Block "%s" is the only block, and cannot be moved' ), type );
return sprintf( __( 'Block "%s" is the only moveable block, and cannot be moved' ), type );
}

if ( dir > 0 && ! isLast ) {
if ( dir > 0 && canMoveDown ) {
// moving down
return sprintf(
__( 'Move "%(type)s" block from position %(position)d down to position %(newPosition)d' ),
Expand All @@ -40,13 +40,13 @@ export function getBlockMoverLabel( selectedCount, type, firstIndex, isFirst, is
);
}

if ( dir > 0 && isLast ) {
if ( dir > 0 && ! canMoveDown ) {
// moving down, and is the last item
// translators: %s: Type of block (i.e. Text, Image etc)
return sprintf( __( 'Block "%s" is at the end of the content and can’t be moved down' ), type );
}

if ( dir < 0 && ! isFirst ) {
if ( dir < 0 && canMoveUp ) {
// moving up
return sprintf(
__( 'Move "%(type)s" block from position %(position)d up to position %(newPosition)d' ),
Expand All @@ -58,10 +58,10 @@ export function getBlockMoverLabel( selectedCount, type, firstIndex, isFirst, is
);
}

if ( dir < 0 && isFirst ) {
if ( dir < 0 && ! canMoveUp ) {
// moving up, and is the first item
// translators: %s: Type of block (i.e. Text, Image etc)
return sprintf( __( 'Block "%s" is at the beginning of the content and can’t be moved up' ), type );
return sprintf( __( 'Block "%s" is at the beginning of moveable blocks and can’t be moved up' ), type );
}
}

Expand All @@ -70,24 +70,24 @@ export function getBlockMoverLabel( selectedCount, type, firstIndex, isFirst, is
*
* @param {number} selectedCount Number of blocks selected.
* @param {number} firstIndex The index (position - 1) of the first block selected.
* @param {boolean} isFirst This is the first block.
* @param {boolean} isLast This is the last block.
* @param {boolean} canMoveUp Indicates whether the first selected block can move up.
* @param {boolean} canMoveDown Indicates whether the last selected block can move down.
* @param {number} dir Direction of movement (> 0 is considered to be going
* down, < 0 is up).
* @return {string} Label for the block movement controls.
*/
export function getMultiBlockMoverLabel( selectedCount, firstIndex, isFirst, isLast, dir ) {
export function getMultiBlockMoverLabel( selectedCount, firstIndex, canMoveUp, canMoveDown, dir ) {
const position = ( firstIndex + 1 );

if ( dir < 0 && isFirst ) {
if ( dir < 0 && ! canMoveUp ) {
return __( 'Blocks cannot be moved up as they are already at the top' );
}

if ( dir > 0 && isLast ) {
if ( dir > 0 && ! canMoveDown ) {
return __( 'Blocks cannot be moved down as they are already at the bottom' );
}

if ( dir < 0 && ! isFirst ) {
if ( dir < 0 && canMoveUp ) {
return sprintf(
__( 'Move %(selectedCount)d blocks from position %(position)d up by one place' ),
{
Expand All @@ -97,7 +97,7 @@ export function getMultiBlockMoverLabel( selectedCount, firstIndex, isFirst, isL
);
}

if ( dir > 0 && ! isLast ) {
if ( dir > 0 && canMoveDown ) {
return sprintf(
__( 'Move %(selectedCount)d blocks from position %(position)s down by one place' ),
{
Expand Down
11 changes: 9 additions & 2 deletions editor/effects.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { get, uniqueId } from 'lodash';
/**
* WordPress dependencies
*/
import { parse, getBlockType, switchToBlockType } from '@wordpress/blocks';
import { parse, getBlockType, switchToBlockType, createBlock } from '@wordpress/blocks';
import { __ } from '@wordpress/i18n';

/**
Expand Down Expand Up @@ -254,12 +254,15 @@ export default {
SET_INITIAL_POST( action ) {
const { post } = action;
const effects = [];
let blocks = [];

// Parse content as blocks
if ( post.content.raw ) {
effects.push( resetBlocks( parse( post.content.raw ) ) );
blocks = blocks.concat( parse( post.content.raw ) );
}

effects.push( resetBlocks( blocks ) );

// Resetting post should occur after blocks have been reset, since it's
// the post reset that restarts history (used in dirty detection).
effects.push( resetPost( post ) );
Expand All @@ -273,4 +276,8 @@ export default {

return effects;
},
RESET_BLOCKS( action, store ) {
const blocks = [ createBlock( 'core/post-title' ) ].concat( action.blocks );
store.dispatch( { type: 'ACTUALLY_RESET_BLOCKS', blocks } );
Copy link
Member

Choose a reason for hiding this comment

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

If we need to better distinguish between effects triggered by SET_INITIAL_POST and what is considered in resetting blocks, I think we should either come up with better names (avoid "ACTUALLY_" prefix) or a different pattern for manipulating how post title is inserted into the block set. A standalone middleware which modifies action.blocks before dispatch occurs is one option.

},
};
Loading