Quote Block: Support Block Switcher #484

youknowriad · 2017-04-24T08:30:13Z

This PR adds the support for the block switcher to the quote block.

Question:

While switching from a quote to heading, this PR is taking the first Paragraph of the quote only, we'll lose the other paragraphs and the citation. We should take a decision whether or not this is acceptable. The alternatives are:

concat all the paragraphs and the citation with a space.
disable this transformation.
We should take a decision here to make this behaviour consistent for all the "Multi-paragraph" to "Single paragraph" switches. @jasmussen. This is also addressed in Add quote↔text block switching; add block switcher validation #465 for the text block.

jasmussen · 2017-04-24T08:46:55Z

Works well, nice work!

While switching from a quote to heading, this PR is taking the first Paragraph of the quote only.

How hard would it be to convert multiple paragraphs in a quote to multiple headings?

By the way I can't get the citation to save. Probably not part of this PR but thought I'd mention.

youknowriad · 2017-04-24T08:55:33Z

How hard would it be to convert multiple paragraphs in a quote to multiple headings?

This is doable, not that hard, we could imagine that returning an array from a transformation means we're creating multiple blocks. But I don't know how I feel about it, I feel it's a not really necessary to add this complexity to the API. Could this be confusing a bit for the user?

Also, this can have implications on the merge API #461. Curious to have others (@aduth @nylen) opinion on this.

By the way I can't get the citation to save. Probably not part of this PR but thought I'd mention.

This is happening when editing an empty citation right? I'll try to fix this in a separate PR.

jasmussen · 2017-04-24T09:04:41Z

Regardless of implementation, it seems like this is one of those cases that suggests we might need to default to text being a continuous block (#447) cc @mtias.

This is happening when editing an empty citation right? I'll try to fix this in a separate PR.

Correct, and thank you!

youknowriad · 2017-04-24T09:29:48Z

those cases that suggests we might need to default to text being a continuous block

I have mixed filling concerning the continuous block, it feels like we'll be adding a global TinyMCE prototype inside the per-block prototype. We'll need to duplicate the work being done for all the different APIs (switching, controls, moving, ...).

mtias · 2017-04-24T09:33:36Z

We'll need to duplicate the work being done for all the different APIs (switching, controls, moving, ...).

These are things the tiny group is working on regardless, we'll see how that coalesces.

aduth · 2017-04-24T14:50:40Z

blocks/library/quote/index.js

+			{
+				type: 'block',
+				blocks: [ 'core/heading' ],
+				transform: ( { content = '' } ) => {


content is no longer a string as of #466

waiting for concatChildren from #461 to fix this

aduth · 2017-04-24T14:51:12Z

blocks/library/quote/index.js

+				blocks: [ 'core/text' ],
+				transform: ( { value, citation } ) => {
+					let content = value ? value : [];
+					content = citation && citation.trim() ? content.concat( citation ) : content;


Similarly, content and citation are no longer strings here.

ellatrix · 2017-04-24T16:15:46Z

While switching from a quote to heading, this PR is taking the first Paragraph of the quote only, we'll lose the other paragraphs and the citation. We should take a decision whether or not this is acceptable.

IMHO, we shouldn't remove the content. Maybe the citation can be added as another paragraph to the blockquote paragraphs?

nylen · 2017-04-26T13:07:24Z

Big -1 to any block switch removing content. I still think it would be better to disable the switch with an appropriate warning message indicating how to resolve the data loss, as I tried in #465.

youknowriad · 2017-04-26T15:59:26Z

Maybe the citation can be added as another paragraph to the blockquote paragraphs?

I'll update to append the citation as another block. Just waiting for #461 which provides concatChildren.

The reason I chose to take the first paragraph only was consistency with the text block. I'll update both.

youknowriad · 2017-04-27T09:24:21Z

PR updated, Quote to heading and text to heading concat paragraph content instead of picking the first paragraph to avoid data loss.

Rebased and used concatChildren

aduth · 2017-04-27T18:31:56Z

blocks/library/heading/index.js

-						// TODO reject the switch if more than one paragraph
-						content = content[ 0 ];
+						content = wp.element.concatChildren( content.map( ( elt ) =>
+							! elt || isString( elt ) || elt.type !== 'p' ? elt : elt.props.children


Quite unfortunate that the heading transform needs to be aware for text forms as an array, as a falsey value, as a string, as an element of paragraph type, or other element type 😕

nylen · 2017-04-27T18:34:12Z

Quote to heading and text to heading concat paragraph content instead of picking the first paragraph to avoid data loss.

In order to understand what is going on here, it's necessary to have a pretty deep understanding of how the editor works, and this isn't something that we can expect of users. Especially with longer content, the current result is still a pretty poor user experience:

Text block

Switch to heading block

Switch back to text block

Alternatives

Transform only the current paragraph into a heading. Split the current block into two (if the current paragraph is the first or the last) or three (if it's somewhere in the middle). I think this would be the least surprising UX but probably also the most difficult to implement.
Disable the switch with a message indicating how to resolve it (Add quote↔text block switching; add block switcher validation #465, Quote Block: Support Block Switcher #484 (comment)). I still like this approach because we should detect operations that will have unintended results and allow the user to clarify their intent.
Convert multiple paragraphs in a quote to multiple headings (Quote Block: Support Block Switcher #484 (comment)). I agree this is a pretty surprising user experience as well.

jasmussen · 2017-04-28T07:35:06Z

From James' alternatives above, I like option 1 and 3 (split into one heading and succeeding paragraphs & convert multiple headings).

youknowriad · 2017-04-28T09:56:07Z

In c040210 I've implemented the proposal 3 from James. So now a block author can return an array from a transformation which will create several blocks of the same type.

I think the proposal 1 is better but if we want to build it, we have two choices:

Change the transform return value to include blockTypes
Assume that if a transform returns an array, the first value of the array is the switched block, the other are keeping the same blockType as the transformed block.

I'm leaning towards the second, I don't feel strongly about giving that much freedom in choosing block types for the block author but this means the block author is aware that only the first block is the transformed block.

Curious to have your ideas and thoughts on this @aduth @nylen

nylen · 2017-04-28T19:01:37Z

this means the block author is aware that only the first block is the transformed block

If I understand correctly, this won't work... here are the 3 cases we need to handle:

First paragraph is selected and transformed into a heading. The resulting block types are:
[ heading, text ]
One of the middle paragraphs (not the first or last) is selected and transformed into a heading. The resulting block types are:
[ text, heading, text ]
The last paragraph is selected and transformed into a heading. The resulting block types are:
[ text, heading ]

Change the transform return value to include blockTypes

This should be the way forward anyway... it is more explicit. The other option - "Assume that if a transform returns an array, the first value of the array is the switched block" - seems too "magical" to me.

youknowriad · 2017-04-29T08:59:15Z

Trying to think globally here. I think the answer to the following question would help us choose the best answer here (and probably a lot more similar debates):

Do we want to build a good text editor showing different controls depending on the current selection OR Do we want to build an editor based on a solid block concept that is used in different places including the editor

The first assumes the user don't know about the concept of block and in the second one, the user is aware of the block concept.

The answer to this question can be different from a person to another but I strongly think we're doing the second thing where the user is aware of the block concept because the user can insert the blocks manually from the inserter and also, the user can use these blocks outside the editor in the customizer etc...

That said, let's get back to our issue, I think the behavior your proposing @nylen where the block switcher behaves differently depending on the current selection is verry confusing. As a user aware of blocks, I won't understand what's happening. The more I think about it, the more I find that switching a block should produce a unique block, and transforming the text a bit (spaces instead of new paragraphs etc...) is a good tradeoff. As a user, I don't see myself using the switcher a lot in "text" blocks, that's why I'm not too concerned with this. I think the switcher makes a lot of sense for "media" blocks for example where we could use third party blocks of different styling and stuff like that.

That said, I think we should revert to the behavior here #484 (comment) and improve it by adding spaces between the merged paragraphs.

If I try to think as a user not aware of "blocks", I do see the issues you're showing with the examples above, but I just don't think that's the way we should go and we should educate the users about the concept of block because this concept will be central to the WordPress content (same as the widgets, shortcodes etc... now, The user is already aware of those and we're replacing them)

jasmussen · 2017-05-01T08:43:13Z

Just a quick note — tried this today, and I feel like it worked really well and felt predictable. Only hiccup was transforming a single heading to a quote, and back to a heading again — this created a superfluous extra empty heading block below, presumably from the empty cite.

Barring that, I think we should merge this in and keep moving. It works really well as is, and though I can definitely understand the theoretical issues, I see these more like improvements that can be made in the future if during testing we find that they pose a problem.

aduth · 2017-05-01T15:00:04Z

blocks/api/factory.js

+	const transformtionResult = castArray( transformation.transform( block.attributes ) );
+
+	return transformtionResult.map( ( attributes, index ) =>
+		Object.assign( {


Why is this an assign if there's only one argument?

I don't know :P

aduth · 2017-05-01T15:10:12Z

editor/state.js

+				}
+				index = state.indexOf( action.uids[ 0 ] );
+				return state.reduce( ( memo, uid ) => {
+					if ( uid === action.uids[ 0 ] ) {


I see the need to support replacing multiple blocks, but it feels strange that we necessarily assume that if there are multiple blocks to be replaced, they're guaranteed to be in consecutive order. Do you think it would be any better in REPLACE_BLOCKS that we only specify a single uid to be replaced, and in the block.js we call to REMOVE_BLOCK the remainder of the blocks to be replaced?

I'm not assuming they are in a consecutive order, I'm just inserting the new blocks in a consecutive order at the place of the first old block.

But I'm not sure I'll keep this implementation for now. My preference would be to transform one block into another block (1-1) transformation but we'll see with the discussion.

youknowriad · 2017-05-02T11:30:52Z

Per Slack discussion, I updated this to switch only the first paragraph.

We can not set this to be aware of the selected paragraph while applying the switch for now. This needs the current selection to be selected in Redux in a unique way no matter the block type and being able to match this selection against the block attributes.

…ren object - Avoid data loss while switching blocks

…ult object

youknowriad · 2017-05-02T14:58:54Z

@nylen updated and made the blockType explicit in 93b5e40

nylen

I'm still not convinced this is the best UX, but it seems to work as intended, and I understand the pieces aren't in place yet to build what I was thinking of. Also, this approach should be flexible enough to support transformations into an arbitrary array of blocks later on.

I added several other validation checks and unit tests for the new cases introduced here. If all of that looks OK, then let's merge it.

mtias · 2017-05-03T08:45:34Z

blocks/api/factory.js

 	const firstSwitchedBlock = findIndex( transformationResults, ( result ) => result.blockType === blockType );

+	// Ensure that at least one block object returned by the transformation has


Great to see some more comments

nylen · 2017-05-04T03:14:14Z

editor/test/state.js

-		it( 'should insert after the specified block uid', () => {
-			const original = blocks( undefined, {
+		it( 'should replace the selected block', () => {
+			const original = deepFreeze( { uid: 'chicken', typing: false, focus: { editable: 'citation' } } );


I didn't catch this before it was merged, but this really should have been split out into separate lines.

We should use prettier and don't care about this stuff anymore

youknowriad added the [Feature] Blocks Overall functionality of blocks label Apr 24, 2017

youknowriad self-assigned this Apr 24, 2017

youknowriad requested a review from jasmussen April 24, 2017 08:30

aduth previously requested changes Apr 24, 2017

View reviewed changes

youknowriad mentioned this pull request Apr 24, 2017

Switching from text to heading only preserves first paragraph #496

Closed

ellatrix added this to the Prototype Parity milestone Apr 26, 2017

youknowriad force-pushed the add/quote-block-transforms branch from 7c6fa4f to d8d6992 Compare April 27, 2017 09:22

nylen mentioned this pull request Apr 27, 2017

Add quote↔text block switching; add block switcher validation #465

Closed

aduth reviewed Apr 27, 2017

View reviewed changes

aduth reviewed May 1, 2017

View reviewed changes

mtias modified the milestones: May Week 1, Prototype Parity May 1, 2017

youknowriad force-pushed the add/quote-block-transforms branch from c040210 to 48524bc Compare May 2, 2017 11:28

youknowriad added 4 commits May 2, 2017 15:35

Quote Block: Support Block Switcher

ec8c7f6

Quote block: Update transforms since the content is now a React.child…

a2d0d26

…ren object - Avoid data loss while switching blocks

Block Switcher: Allow creating multiple blocks when switching a block

ed9bea6

Block Switcher: Switch only the first paragraph

5469c9a

youknowriad force-pushed the add/quote-block-transforms branch from 48524bc to 5469c9a Compare May 2, 2017 14:36

Block Switcher: Make the blockType explicit in the transformation res…

93b5e40

…ult object

nylen added 8 commits May 2, 2017 18:49

Avoid duplicate slice() call

fa688ad

Ensure that each element of a transformation returns a block type

8b25080

Ensure that at least one transformed block has the expected type

45a2365

Add a test for a successful block array transformation

b2cff33

Simplify the newly added transformation checks

ec4fbe1

Make sure transformation functions return an object or array of objects

f39bcd1

Add some comments

9b4bc88

Fix lint errors

8644d40

nylen approved these changes May 3, 2017

View reviewed changes

youknowriad merged commit 67e2b2b into master May 3, 2017

youknowriad deleted the add/quote-block-transforms branch May 3, 2017 08:19

mtias reviewed May 3, 2017

View reviewed changes

nylen reviewed May 4, 2017

View reviewed changes

nylen mentioned this pull request May 4, 2017

Allow saving and publishing posts #594

Merged

aduth mentioned this pull request Mar 5, 2018

Fixed: Block cannot be removed after turned into another block type #5418

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Quote Block: Support Block Switcher #484

Quote Block: Support Block Switcher #484

youknowriad commented Apr 24, 2017

jasmussen commented Apr 24, 2017

youknowriad commented Apr 24, 2017

jasmussen commented Apr 24, 2017

youknowriad commented Apr 24, 2017 •

edited

Loading

mtias commented Apr 24, 2017

aduth Apr 24, 2017

youknowriad Apr 25, 2017

aduth Apr 24, 2017

ellatrix commented Apr 24, 2017

nylen commented Apr 26, 2017

youknowriad commented Apr 26, 2017

youknowriad commented Apr 27, 2017

aduth Apr 27, 2017

nylen commented Apr 27, 2017 •

edited

Loading

jasmussen commented Apr 28, 2017

youknowriad commented Apr 28, 2017

nylen commented Apr 28, 2017 •

edited

Loading

youknowriad commented Apr 29, 2017 •

edited

Loading

jasmussen commented May 1, 2017

aduth May 1, 2017

youknowriad May 1, 2017

aduth May 1, 2017

youknowriad May 1, 2017

youknowriad commented May 2, 2017 •

edited

Loading

youknowriad commented May 2, 2017

nylen left a comment

mtias May 3, 2017

nylen May 4, 2017

youknowriad May 4, 2017

		const firstSwitchedBlock = findIndex( transformationResults, ( result ) => result.blockType === blockType );

		// Ensure that at least one block object returned by the transformation has

Quote Block: Support Block Switcher #484

Quote Block: Support Block Switcher #484

Conversation

youknowriad commented Apr 24, 2017

jasmussen commented Apr 24, 2017

youknowriad commented Apr 24, 2017

jasmussen commented Apr 24, 2017

youknowriad commented Apr 24, 2017 • edited Loading

mtias commented Apr 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix commented Apr 24, 2017

nylen commented Apr 26, 2017

youknowriad commented Apr 26, 2017

youknowriad commented Apr 27, 2017

Choose a reason for hiding this comment

nylen commented Apr 27, 2017 • edited Loading

Text block

Switch to heading block

Switch back to text block

Alternatives

jasmussen commented Apr 28, 2017

youknowriad commented Apr 28, 2017

nylen commented Apr 28, 2017 • edited Loading

youknowriad commented Apr 29, 2017 • edited Loading

jasmussen commented May 1, 2017

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

youknowriad commented May 2, 2017 • edited Loading

youknowriad commented May 2, 2017

nylen 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

youknowriad commented Apr 24, 2017 •

edited

Loading

nylen commented Apr 27, 2017 •

edited

Loading

nylen commented Apr 28, 2017 •

edited

Loading

youknowriad commented Apr 29, 2017 •

edited

Loading

youknowriad commented May 2, 2017 •

edited

Loading