Add/549 list block indent buttons #717

BoardJames · 2017-05-09T06:23:31Z

The purpose of the pull request is to:

Add indent and outdent buttons to the list block which will apply to the current selection
Allow sublists to have their type changed (unordered to ordered)

Most of the heavy lifting is done by the TinyMCE lists plugin which I've added to the plugins list. It unfortunately introduces a downside in the form of broken tab navigation because it overrides tab to do list indenting. I have submitted a pull request to TinyMCE to work around this by adding a configuration option to the lists plugin.

One of the problems that I encountered was how to execute commands on the editor. Currently I am doing that via the tinymce.activeEditor global but I would like suggestions on a better way of doing this.

youknowriad · 2017-05-09T08:43:45Z

blocks/library/list/index.js

@@ -7,6 +7,34 @@ import Editable from 'components/editable';

 const { children, prop } = hpq;

+function activeEditorExecCommand( command ) {
+	return () => {
+		const ed = tinymce.activeEditor;


I think we'd want to avoid using globals like this. We can't assume TinyMCE is even used for the Editable component here.

Unfortunately, it's very hard to find an alternative here. Maybe the "indent controls" should be considered as "inline toolbars" (the same way we do for formatting and alignment controls)

The problem with that is we're moving code that likely no-one else is going to use into a common object purely to work around access issues. That doesn't appeal to me greatly either.

I managed to avoid using the global but it required passing the editor via a callback.

aduth

I'm inclined to agree with @youknowriad and extend it further to not just accessing the global, but generally moving TinyMCE specific logic out of the Editable component. The goal with blocks and the Editable component is to try to remove the need for block implementers to become familiar with and manually manage TinyMCE APIs atop what they'll already need to learn in creating a block. And more generally that we should limit imperative interactions in favor of declarative paradigms (props passed to describe an Editable, not affecting the change myself by executing commands on the instance).

To your specific needs, we've solved similar (perhaps not identical) requirements by moving logic into the Editable component itself: accepting tagName to reconcile the node type on change, and rendering toolbar controls which affect TinyMCE commands. Your example is a little different in that indenting is quite specific to lists, but I think it might be reasonable to move these into Editable as common controls as well, to be shown when the tagName prop is one of "OL" or "UL".

aduth · 2017-05-10T15:09:54Z

blocks/library/list/index.js

+		{
+			icon: 'editor-outdent',
+			title: wp.i18n.__( 'Outdent list item' ),
+			isActive: () => false,


Is this required to be defined? If so, we should probably update the behavior on rendering controls to allow it to be omitted (defaulting to false).

The isActive is required as editor/modes/visual-editor/blocks.js calls it without checking if it already exists. It would be fairly easy to make it optional.

BoardJames · 2017-05-16T02:07:38Z

@aduth Please re-review. I have tried to implement it as you describe.

Note I don't believe that the current approach of hiding the existence of TinyMCE is going to solve any problems in the long run. Once this is in the wild with people creating their own blocks it will either lead to people reimplementing existing editor functions or accessing the global directly. I am going to have to go through this process of adding block specific functions to the editable again for the table block. This is frustrating to me because the code is not elegant. It loses much that is gained by having separate blocks.

BoardJames · 2017-05-17T00:08:38Z

@youknowriad @aduth Do my changes address your comments correctly? Can I merge?

aduth

This feels much improved to me. From a block implementer's perspective, much of the needed complexity is hidden in Editable itself, and in doing so we ensure consistent behavior of lists across any block that chooses to render one as editable. Admittedly there was a bit more of a challenge in handling "internal" lists than I'd previously realized. I secretly hoped it would be as simple as operating on the Editable's tagName alone. How you've handled this seems reasonable to me though.

Pending conflict resolution and a response about Editable prop naming, this appears mergeable to me.

Once this is in the wild with people creating their own blocks it will either lead to people reimplementing existing editor functions or accessing the global directly.

Sure, I agree lists and tables have pushed the extremes of the abstraction, but I'm personally okay with Editable becoming an unwieldy black box of magic so long as for block implementers we keep its interface is as minimal as possible and ensure consistency between Editables of the same sort. This is just the goal of course, and we'll have to see over time where it falls short, but I'm not content to throw our hands in the air quite yet. And by the refactoring shown here, a little more frustration endured on our parts will hopefully pay off by enabling many others to more easily implement blocks.

aduth · 2017-05-17T12:41:19Z

blocks/library/list/style.scss

@@ -1,3 +1,4 @@
-.blocks-list .blocks-editable__tinymce {
+.blocks-list  .blocks-editable__tinymce, .blocks-list  .blocks-editable__tinymce ul, .blocks-list  .blocks-editable__tinymce ol {


Minor: Can we split individual selectors onto their own lines?

aduth · 2017-05-17T12:48:46Z

blocks/library/list/index.js

-					setAttributes( { values: nextValues } );
-				} }
+				onChange={ ( nextValues ) => setAttributes( { values: nextValues } ) }
+				onListChange={ ( listType ) => setAttributes( { nodeName: listType } ) }


Do you think there's value in naming this prop more generally to reflect that it's a signal from the Editable to change the tagName. Maybe onTagNameChange ?

It's probably not going to be used by any other blocks but I don't mind changing the name.

aduth · 2017-05-17T14:09:48Z

ensure consistency between Editables of the same sort

Part of this assumes we expect blocks rendering their own lists to be a common need. But instead it might be we'd want this to tie into child blocks (not yet implemented), e.g. rendering a List block child. If this were the case, it would be more reasonable for the list block to be specialized in its handling of the controls.

I'd chatted about this with @mtias a bit, and he's of the desire that we don't bloat Editable to cover all cases (at odds with my previous remarks), but instead keep it largely focused at the task of basic inline formatting. There's an obvious challenge in how to build a list block with this goal in mind; a list could be considered as a nested array of items, but it would be extremely difficult to recreate the keyboard behaviors users have come to expect from managing lists.

Maybe we do allow Editable or the underlying TinyMCE component to be extended for these specialized cases. It's unclear to me what exactly this extension behavior looks like (extensibility is largely unsolved). For components, this could take the form of a higher order component, or simply a wrapper component more akin to your original implementation. In both cases, it seems we'd need to embrace the TinyMCE instance being made available so as to be able to bind to its events as you have in Editable currently. This could be located as a common EditableList variant, but if we'd agree lists are not a common behavior to support, maybe it is sensible to simply bake this into the block itself (acknowledging that edit is in-fact a true React component).

Sorry for the back-and-forth on this. It's surfaced a few important questions:

What roles does Editable serve? What are its common baseline behaviors to ensuring consistency across blocks?
How do we extend components for specialized cases to keep the base implementation lean?

aduth · 2017-05-17T18:01:06Z

I've been fumbling around with ideas, since I don't want to leave this without an actionable direction. I'm curious to hear how you and others feel because I'm still feeling uncertain, but I think a good initial solution could be one more in the middle between what exists in the last commit of the branch, and your original approach. There's a few general goals I have in mind:

Avoid the tinymce.activeEditor global
Override settings to provide lists plugin, not as default plugin to all Editables
Surface the TinyMCE instance directly for these unique cases, and bind to events on the instance (instead of as individual props on Editable)

Much of this seems to involve bringing up what's implemented here now in Editable and TinyMCE to the list block, and exposing a few new extension props on Editable. In this way more as a specialized Editable specific to the list block.

Roughly, this could look something like: https://gist.github.com/aduth/9476aeaf6f779799149f5e7797da0671

This process has illustrated to me a few pain points:

Customizing controls based on the component instance is cumbersome, using <Fill /> directly instead of the block's own controls property. We should look to bring these closer together.
- Pass component instance to control callbacks? Bind this of control callbacks to component instance? Define controls as a method or static member on the edit Component class? Eliminate controls property of the block and instead default to a <Fill /> pattern in edit's rendering?
We really need extension patterns not only for Fill-ing, but also wrapping components and overriding default data (like settings)

BoardJames · 2017-05-18T23:58:09Z

@aduth my second draft (after the removal of the global access but before I moved everything into Editable, essentially the first thing you reviewed.) does almost exactly what your list.js seems to be doing. If you are allowing the editor to escape the Editable what does making the new list.js react component gain?

BoardJames · 2017-05-19T00:49:28Z

@aduth The documentation you link says that I should not put HOC code in edit because that is called directly by render which apparently messes up the diffing algorithm.
https://facebook.github.io/react/docs/higher-order-components.html#dont-use-hocs-inside-the-render-method

This reverts commit abb531f.

This reverts commit f60e11d.

BoardJames · 2017-05-19T05:59:54Z

I reverted to the original version where the list code is not in Editable and tried to modify it to behave like you list.js. It now adds an event handler on the editor for nodeChange and modifies the editor configuration using onConfig.

aduth

I think we can merge after the following changes, but feel free to iterate based on other comments:

Remove unused onNodeChange prop handling in Editable
Decide between onConfig as mutator or getSettings as immutable settings merge getter

aduth · 2017-05-19T15:18:39Z

blocks/library/list/index.js

+						setAttributes( { internalListType: findInternalListType( nodeInfo ) } );
+					} );
+					setAttributes( { editor } );
+				}	}


Tab snuck in instead of space between curly braces.

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.

aduth · 2017-05-19T15:24:36Z

blocks/library/list/index.js

+				onConfig={ ( settings ) => ( {
+					...settings,
+					plugins: ( settings.plugins || [] ).concat( 'lists' ),
+					lists_indent_on_tab: false,


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.

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.

aduth · 2017-05-19T15:28:19Z

blocks/editable/index.js

@@ -438,12 +453,10 @@ export default class Editable extends wp.element.Component {

 				<TinyMCE
 					tagName={ tagName }
+					onConfig={ this.onConfig }


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 } { /* ... */ } /> );

Good idea. I will do that.

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.

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.

aduth · 2017-05-19T15:28:39Z

blocks/editable/index.js

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


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.

I just kept the existing behaviour.

aduth · 2017-05-19T15:29:52Z

blocks/editable/index.js

@@ -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 ) {


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.

aduth · 2017-05-19T15:35:24Z

blocks/library/list/index.js

+}
+
+function findInternalListType( { parents } ) {
+	const list = parents.find( ( node ) => node.nodeName === 'UL' || node.nodeName === 'OL' );


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';.

Ok, I changed it to use the lodash find.

aduth · 2017-05-19T15:40:27Z

blocks/library/list/index.js

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


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>

I changed onConfig() to getSettings() .

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

aduth · 2017-05-19T15:45:29Z

blocks/library/list/index.js

+					editor.on( 'nodeChange', ( nodeInfo ) => {
+						setAttributes( { internalListType: findInternalListType( nodeInfo ) } );
+					} );
+					setAttributes( { editor } );


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:



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.

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.

aduth · 2017-05-19T15:50:45Z

If you are allowing the editor to escape the Editable what does making the new list.js react component gain?

See #717 (comment)

The documentation you link says that I should not put HOC code in edit because that is called directly by render which apparently messes up the diffing algorithm.

Yeah, the docs make sense, but what did you worry was wrapping an HoC in render?

BoardJames · 2017-05-22T02:05:54Z

As requested I have addressed the key points:

Remove unused onNodeChange prop handling in Editable
Decide between onConfig as mutator or getSettings as immutable settings merge getter
I have also
Replaced collection.find with lodash's find
Fixed tabs in the wrong places
I think it's ready to merge.

tiny-james added 3 commits May 9, 2017 15:57

Add lists plugin to support list indenting correctly

40472da

Add onNodeChange as an optional entry in the Editable props

b19ee7f

Add indent/outdent buttons; list type buttons handle sublists

2418990

BoardJames mentioned this pull request May 9, 2017

List: implement levels of indent buttons #549

Closed

youknowriad reviewed May 9, 2017

View reviewed changes

tiny-james added 2 commits May 10, 2017 10:20

Merge branch 'master' into add/549-list-block-indent-buttons

d5c0d0b

Avoid use of global tinymce.activeEditor

5f2c3d0

aduth requested changes May 10, 2017

View reviewed changes

tiny-james added 6 commits May 11, 2017 10:31

Merge branch 'master' into add/549-list-block-indent-buttons

390e99a

Allow the control.isActive function to be optional defaulting to false

ba33927

Merge branch 'master' into add/549-list-block-indent-buttons

8dfdd74

Merge branch 'master' into add/549-list-block-indent-buttons

1d93fbc

Merge branch 'master' into add/549-list-block-indent-buttons

4cc20bf

Moved list logic into Editable

f60e11d

Removed onSetup and onNodeChange callbacks

abb531f

Merge branch 'master' into add/549-list-block-indent-buttons

17a5168

aduth reviewed May 17, 2017

View reviewed changes

aduth mentioned this pull request May 17, 2017

Try: Block controls rendering by block's edit #830

Merged

tiny-james added 2 commits May 18, 2017 09:26

Merge branch 'master' into add/549-list-block-indent-buttons

1104141

Put separate CSS selectors on different lines to improve readablity

27e1c0f

tiny-james added 4 commits May 19, 2017 13:56

Merge branch 'master' into add/549-list-block-indent-buttons

7156082

Revert "Removed onSetup and onNodeChange callbacks"

f8cce5a

This reverts commit abb531f.

Revert "Moved list logic into Editable"

befe07b

This reverts commit f60e11d.

Removed onNodeChange; allow tinyMCE configuration outside Editable

528f088

aduth requested changes May 19, 2017

View reviewed changes

tiny-james added 5 commits May 22, 2017 10:34

Merge branch 'master' into add/549-list-block-indent-buttons

05288e1

Remove onNodeChange property callback

68ab9ee

Renamed onConfig to getSettings

8e36e05

Use find from lodash to avoid IE 11 support problems

9c5b029

Replaced some out of position tab chars with spaces

9ed5cf4

BoardJames merged commit cfd4d94 into master May 22, 2017

BoardJames deleted the add/549-list-block-indent-buttons branch May 22, 2017 02:08

		@@ -1,3 +1,4 @@
		.blocks-list .blocks-editable__tinymce {
		.blocks-list .blocks-editable__tinymce, .blocks-list .blocks-editable__tinymce ul, .blocks-list .blocks-editable__tinymce ol {

Add/549 list block indent buttons #717

Add/549 list block indent buttons #717

Conversation

BoardJames commented May 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BoardJames commented May 16, 2017

BoardJames commented May 17, 2017

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented May 17, 2017

aduth commented May 17, 2017

BoardJames commented May 18, 2017

BoardJames commented May 19, 2017

BoardJames commented May 19, 2017

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BoardJames May 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented May 19, 2017

BoardJames commented May 22, 2017

BoardJames May 22, 2017 •

edited

Loading