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.

Sign up for GitHub

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

Jump to bottom

Add/549 list block indent buttons #717

Merged
merged 24 commits into from
May 22, 2017
Merged

Add/549 list block indent buttons #717

Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
40472da
Add lists plugin to support list indenting correctly
tiny-james May 9, 2017
b19ee7f
Add onNodeChange as an optional entry in the Editable props
tiny-james May 9, 2017
2418990
Add indent/outdent buttons; list type buttons handle sublists
tiny-james May 9, 2017
d5c0d0b
Merge branch 'master' into add/549-list-block-indent-buttons
tiny-james May 10, 2017
5f2c3d0
Avoid use of global tinymce.activeEditor
tiny-james May 10, 2017
390e99a
Merge branch 'master' into add/549-list-block-indent-buttons
tiny-james May 11, 2017
ba33927
Allow the control.isActive function to be optional defaulting to false
tiny-james May 11, 2017
8dfdd74
Merge branch 'master' into add/549-list-block-indent-buttons
tiny-james May 12, 2017
1d93fbc
Merge branch 'master' into add/549-list-block-indent-buttons
tiny-james May 15, 2017
4cc20bf
Merge branch 'master' into add/549-list-block-indent-buttons
tiny-james May 15, 2017
f60e11d
Moved list logic into Editable
tiny-james May 16, 2017
abb531f
Removed onSetup and onNodeChange callbacks
tiny-james May 16, 2017
17a5168
Merge branch 'master' into add/549-list-block-indent-buttons
tiny-james May 17, 2017
1104141
Merge branch 'master' into add/549-list-block-indent-buttons
tiny-james May 17, 2017
27e1c0f
Put separate CSS selectors on different lines to improve readablity
tiny-james May 17, 2017
7156082
Merge branch 'master' into add/549-list-block-indent-buttons
tiny-james May 19, 2017
f8cce5a
Revert "Removed onSetup and onNodeChange callbacks"
tiny-james May 19, 2017
befe07b
Revert "Moved list logic into Editable"
tiny-james May 19, 2017
528f088
Removed onNodeChange; allow tinyMCE configuration outside Editable
tiny-james May 19, 2017
05288e1
Merge branch 'master' into add/549-list-block-indent-buttons
tiny-james May 22, 2017
68ab9ee
Remove onNodeChange property callback
tiny-james May 22, 2017
8e36e05
Renamed onConfig to getSettings
tiny-james May 22, 2017
9c5b029
Use find from lodash to avoid IE 11 support problems
tiny-james May 22, 2017
9ed5cf4
Replaced some out of position tab chars with spaces
tiny-james May 22, 2017
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
23 changes: 18 additions & 5 deletions blocks/editable/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* External dependencies
*/
import classnames from 'classnames';
import { last, isEqual, capitalize, omitBy, forEach, merge } from 'lodash';
import { last, isEqual, capitalize, omitBy, forEach, merge, identity } from 'lodash';
import { nodeListToReact } from 'dom-react';
import { Fill } from 'react-slot-fill';
import 'element-closest';
Expand Down Expand Up @@ -67,6 +67,7 @@ export default class Editable extends wp.element.Component {
super( ...arguments );

this.onInit = this.onInit.bind( this );
this.onConfig = this.onConfig.bind( this );
this.onSetup = this.onSetup.bind( this );
this.onChange = this.onChange.bind( this );
this.onNewBlock = this.onNewBlock.bind( this );
Expand All @@ -84,6 +85,13 @@ export default class Editable extends wp.element.Component {
};
}

onConfig( settings ) {
return ( this.props.onConfig || identity )( {
...settings,
forced_root_block: this.props.inline ? false : 'p',
Copy link
Member

Choose a reason for hiding this comment

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

Should our forced_root_block always take precedence over the block's own? I don't really feel strongly either way, just seems like a block overriding settings might want ability to override anything.

Copy link
Author

Choose a reason for hiding this comment

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

I just kept the existing behaviour.

} );
}

onSetup( editor ) {
this.editor = editor;
editor.on( 'init', this.onInit );
Expand All @@ -93,6 +101,10 @@ export default class Editable extends wp.element.Component {
editor.on( 'nodechange', this.onNodeChange );
editor.on( 'keydown', this.onKeyDown );
editor.on( 'selectionChange', this.onSelectionChange );

if ( this.props.onSetup ) {
this.props.onSetup( editor );
}
}

onInit() {
Expand Down Expand Up @@ -273,6 +285,10 @@ export default class Editable extends wp.element.Component {
const focusPosition = this.getRelativePosition( element );
const bookmark = this.editor.selection.getBookmark( 2, true );
this.setState( { alignment, bookmark, formats, focusPosition } );

if ( this.props.onNodeChange ) {
Copy link
Member

Choose a reason for hiding this comment

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

We can safely remove this, as in your implementation we've exposed the editor instance via onSetup and bound to the event directly in the block.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

this.props.onNodeChange( { element, parents } );
}
}

updateContent() {
Expand Down Expand Up @@ -394,7 +410,6 @@ export default class Editable extends wp.element.Component {
className,
showAlignments = false,
inlineToolbar = false,
inline,
formattingControls,
placeholder,
} = this.props;
Expand Down Expand Up @@ -438,12 +453,10 @@ export default class Editable extends wp.element.Component {

<TinyMCE
tagName={ tagName }
onConfig={ this.onConfig }
Copy link
Member

Choose a reason for hiding this comment

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

Since we only ever use this in <TinyMCE /> for initializing the instance, could we simply prepare the settings ahead of time and avoid the callback?

Thinking something like:

let settings = {
	forced_root_block: this.props.inline ? false : 'p'
};

if ( this.props.onConfig ) {
	settings = this.props.onConfig( settings );
}

return (
	<TinyMCE
		settings={ settings }
		{ /* ... */ }
	/>
);

Copy link
Author

Choose a reason for hiding this comment

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

Good idea. I will do that.

Copy link
Author

@BoardJames BoardJames May 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, the problem is that without the callback we have to put any code for merging settings (ie for example the list of plugins loaded) into the TinyMCE block. The callback is rare anyway so it's not worth optimizing.

Copy link
Author

Choose a reason for hiding this comment

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

Either that or the default TinyMCE settings would have to be some sort of constant accessible outside of the block so the wrapping blocks can handle the settings merge. I am not sure how to go about doing that.

onSetup={ this.onSetup }
style={ style }
defaultValue={ value }
settings={ {
forced_root_block: inline ? false : 'p',
} }
isEmpty={ this.state.empty }
placeholder={ placeholder }
key={ key } />
Expand Down
17 changes: 10 additions & 7 deletions blocks/editable/tinymce.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,24 +34,27 @@ export default class TinyMCE extends wp.element.Component {
}

initialize() {
const { settings, focus } = this.props;
const { focus } = this.props;

tinymce.init( {
target: this.editorNode,
const settings = this.props.onConfig( {
theme: false,
inline: true,
toolbar: false,
browser_spellcheck: true,
entity_encoding: 'raw',
convert_urls: false,
setup: ( editor ) => {
this.editor = editor;
this.props.onSetup( editor );
},
formats: {
strikethrough: { inline: 'del' },
},
} );

tinymce.init( {
...settings,
target: this.editorNode,
setup: ( editor ) => {
this.editor = editor;
this.props.onSetup( editor );
},
} );

if ( focus ) {
Expand Down
67 changes: 59 additions & 8 deletions blocks/library/list/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,40 @@ import Editable from '../../editable';

const { children, prop } = hpq;

function execCommand( command ) {
return ( { editor } ) => {
if ( editor ) {
editor.execCommand( command );
}
};
}

function listIsActive( listType ) {
return ( { nodeName = 'OL', internalListType } ) => {
return listType === ( internalListType ? internalListType : nodeName );
};
}

function listSetType( listType, editorCommand ) {
return ( { internalListType, editor }, setAttributes ) => {
if ( internalListType ) {
// only change list types, don't toggle off internal lists
if ( internalListType !== listType ) {
if ( editor ) {
editor.execCommand( editorCommand );
}
}
} else {
setAttributes( { nodeName: listType } );
}
};
}

function findInternalListType( { parents } ) {
const list = parents.find( ( node ) => node.nodeName === 'UL' || node.nodeName === 'OL' );
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 currently use a polyfill for prototype methods on base object types, so Array#find, String#includes et. all will fail in Internet Explorer 11. I think we will want a polyfill, mostly because it's confusing for developers to have access to some but not all ES2015+ features. I'd started to explore this in #746, but closed it in anticipation of an upcoming babel-preset-env "usage" feature.

...which is all to say, this will probably be fine, but cause Internet Explorer to fail for some time. Alternative is to use import { find } from 'lodash';.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I changed it to use the lodash find.

return list ? list.nodeName : null;
}

registerBlock( 'core/list', {
title: wp.i18n.__( 'List' ),
icon: 'editor-ul',
Expand All @@ -26,18 +60,24 @@ registerBlock( 'core/list', {
{
icon: 'editor-ul',
title: wp.i18n.__( 'Convert to unordered' ),
isActive: ( { nodeName = 'OL' } ) => nodeName === 'UL',
onClick( attributes, setAttributes ) {
setAttributes( { nodeName: 'UL' } );
},
isActive: listIsActive( 'UL' ),
onClick: listSetType( 'UL', 'InsertUnorderedList' ),
},
{
icon: 'editor-ol',
title: wp.i18n.__( 'Convert to ordered' ),
isActive: ( { nodeName = 'OL' } ) => nodeName === 'OL',
onClick( attributes, setAttributes ) {
setAttributes( { nodeName: 'OL' } );
},
isActive: listIsActive( 'OL' ),
onClick: listSetType( 'OL', 'InsertOrderedList' ),
},
{
icon: 'editor-outdent',
title: wp.i18n.__( 'Outdent list item' ),
onClick: execCommand( 'Outdent' ),
},
{
icon: 'editor-indent',
title: wp.i18n.__( 'Indent list item' ),
onClick: execCommand( 'Indent' ),
},
],

Expand Down Expand Up @@ -72,6 +112,17 @@ registerBlock( 'core/list', {
return (
<Editable
tagName={ nodeName.toLowerCase() }
onConfig={ ( settings ) => ( {
Copy link
Member

Choose a reason for hiding this comment

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

I like that this is immutable, but it does seem a bit odd to have an on- prefixed callback expected to return a value. I think we could choose between keeping this as onConfig and mutating settings, or changing the name to something like getSettings.

Or, and forgive me again for my extreme pondering, explore functions as children as an interesting extensibility option for overriding the default TinyMCE rendering to extend settings. Something like:

<Editable
	{ /* ... */ }
>
	( tinyMCEProps ) => (
		<TinyMCE 
			{ ...tinyMCEProps }
			settings={
				...tinyMCEProps.settings,
				plugins: ( settings.plugins || [] ).concat( 'lists' ),
				lists_indent_on_tab: false,
			}
		/>
	)
</Editable>

Copy link
Author

Choose a reason for hiding this comment

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

I changed onConfig() to getSettings() .

I don't understand what you are trying to explain with the code snippet.

...settings,
plugins: ( settings.plugins || [] ).concat( 'lists' ),
lists_indent_on_tab: false,
Copy link
Member

Choose a reason for hiding this comment

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

I know you noted this in the original pull request description. It did catch me off guard though; I'd tried tabbing to indent and the screen shifted to focus the next block. I don't know that I have a good answer here. Perhaps @jasmussen could weigh in to UX implications, but we could keep this as-is for now to at least allow consistent tab behavior.

Copy link
Author

Choose a reason for hiding this comment

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

In normal TinyMCE you can escape the list by using the arrow keys and then tabbing works as normal. The problem is that because TinyMCE does not extend out of the list in this case you can't escape the list block and hence it is impossible to navigate away only using the keyboard.

It is a tricky problem, possibly another keybinding could be used for indenting or possibly the arrow keys could be used for inter-block navigation when you are on the first/last line.

} ) }
onSetup={ ( editor ) => {
editor.on( 'nodeChange', ( nodeInfo ) => {
setAttributes( { internalListType: findInternalListType( nodeInfo ) } );
} );
setAttributes( { editor } );
Copy link
Member

Choose a reason for hiding this comment

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

I haven't really settled on whether we should encourage attributes as a generic object store. One issue and potential reason for not leaning on it heavily is that the default behavior of serializing block comments is to encode any additional attributes outside a block's own attributes property into the comment itself. You can see this when changing from Visual to Text, that the block separation includes editor and internalListType:

<!-- wp:core/list editor="[object Object]" internalListType="null" -->

The proposed block API includes an encodeAttributes function which was intended to address this concern, allowing block implementers to omit properties not desired to be included in the comment. This has not yet been implemented though. If it were, I might suggest that it be an opt-in than an opt-out to the comment attributes.

What do you think?

We could leave this for now and address separately. This is also why I'd considered using state in the edit: class extends Component gist for editor and internal list type.

Copy link
Author

Choose a reason for hiding this comment

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

I'll leave it for now.

I was unaware that state was being serialized beyond what I was outputting in save. I think it should be opt-in.

} }
Copy link
Member

Choose a reason for hiding this comment

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

Tab snuck in instead of space between curly braces.

Copy link
Author

Choose a reason for hiding this comment

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

Hmmm, I have my editor set to use spaces but the the ESLint plugin converts them to tabs. I suspect that in this case it was split over multiple lines and I collapsed it to one line - so the tab got where it shouldn't.

onChange={ ( nextValues ) => {
setAttributes( { values: nextValues } );
} }
Expand Down
5 changes: 4 additions & 1 deletion blocks/library/list/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
.blocks-list .blocks-editable__tinymce {
.blocks-list .blocks-editable__tinymce,
.blocks-list .blocks-editable__tinymce ul,
.blocks-list .blocks-editable__tinymce ol {
padding-left: 2.5em;
margin-left: 0;
}
2 changes: 1 addition & 1 deletion editor/modes/visual-editor/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ class VisualEditorBlock extends wp.element.Component {
controls={ settings.controls.map( ( control ) => ( {
...control,
onClick: () => control.onClick( block.attributes, this.setAttributes ),
isActive: control.isActive( block.attributes ),
isActive: control.isActive ? control.isActive( block.attributes ) : false,
} ) ) } />
) }
<Slot name="Formatting.Toolbar" />
Expand Down