Quote Block: Adding alignment controls to the quote block

[youknowriad]

This PR adds the alignment controls to the quote block #311

It also allows the block controls to be defined as an array of array. This creates multiple toolbars. Each array is a different toolbar.

[aduth]

Do we want the text alignment to apply to the entire quote (including citation), or just the paragraphs? cc @jasmussen

[joyously]

The text alignment only applies to the selected text, not a block. Float alignment applies to the whole block.

[youknowriad]

Good question @joyously and @jasmussen might help to answer here. For me considering the alignment as inline controls is relevant for the continuous Text block #447.

[jasmussen]

Sorry, I missed this. Yes it would be preferrable if we were able to have the alignments be per paragraph. Let me know if you'd like a mockup.

[youknowriad]

Can I ask for a clarification? Is this for the current quote block or for the continuous Text block? Yep, some mockups could be good :)

[jasmussen]

Sorry for the midstream correction on this one. Here's a mockup of a multi paragraph quote:

Note how the caret is in text that is left aligned, therefore the toolbar shows the left alignment highlighted.

Now the first paragraph is right aligned:

Note how the caret is inside the first paragraph.

Technically the behavior here would be the same as that of the "Continuous text" block.

[aduth]

Ah, I had fun with this one, but alas, no obvious Lodash-fu will save it 😄

This is the closest I got, which has a few advantages; handles undefined controls, gracefully handles controls of mixed types (instead of testing first entry only), and always returns an array, even if empty, so that we don't need to test truthiness before mapping in the render below. But it's a little complex to behold.

reduce( settings.controls, ( memo, toolbar ) => (
	Array.isArray( toolbar )
		? [ ...memo, toolbar ]
		: [ ...memo.slice( 0, -1 ), [ ...( last( memo ) || [] ), toolbar ) ] ]
), [] );

At the very least, I'm not sure we need first, since we can be certain the array exists and has members at that point in the condition. Simply referencing the [ 0 ] entry should be enough.

[youknowriad]

I'm not sure I understand the magic here :P but it seems that if we have mixed values, the controls outside arrays are included in the previous array, which I think is not the desired behaviour if we ever have mixed controls. I'd think these "orphans" should be grouped together.

Yes. I'll drop the first, it seems enough for now to have only Arrays or only orphans

[aduth]

if we have mixed values, the controls outside arrays are included in the previous array, which I think is not the desired behaviour if we ever have mixed controls

I don't know that we'd want to encourage this anyways, but in the current implementation this will simply explode (trying to map on a non-array); thus the "graceful" remark.

[aduth]

Appears these got doubled up. Another npm run build should clear it up. Not certain what causes this.

[aduth]

I added some clarifying comments in bd4e50f38b12090fe112ef73170160b4d615136b . It is maybe an overly complex solution, but I'm generally reluctant about logic which makes assumptions using only the first of potentially many entries in an array. Could maybe do for some tests too if we keep this.

[aduth]

Alternatively we could be a little less forgiving in the handling of singular values:

diff --git a/editor/modes/visual-editor/block.js b/editor/modes/visual-editor/block.js
index 76b9f2a..e60d117 100644
--- a/editor/modes/visual-editor/block.js
+++ b/editor/modes/visual-editor/block.js
@@ -142,7 +142,7 @@ class VisualEditorBlock extends wp.element.Component {
                                ? [ ...result, toolbar ]
                                // Singular values are appended to the last array in the result
                                //  Example: [ 1, 2, 3, 4 ] => [ [ 1, 2, 3, 4 ] ]
-                               : [ ...result.slice( 0, -1 ), [ ...( last( result ) || [] ), toolbar ] ]
+                               : [ [ ...( result[ 0 ] || [] ), toolbar ] ]
                ), [] );
 
                // Disable reason: Each block can receive focus but must be able to contain

[ellatrix]

Maybe something like <div className="editor-toolbar__group"> should move to a separate component?

[youknowriad]

Yep, but this component will have the following props : content, attributes, setAttributes. Or we could pass a block uid instead and duplicate setAttributes. Having attributes and setAttributes as props for a GroupedToolbars might not be a good idea.

[youknowriad]

Per Slack Discussion, I've reverted to toolbars normalisation. Let me know if there's anything else here?

[aduth]

This comment chain still seems to be unresolved:

#511 (comment)

(Or at least lacking clarification)

[aduth]

This will error if a block doesn't define controls because toolbars will be undefined. We'll want to restore the truthy test or use Lodash's map.