Blocks: Fix quote block (parsing, serializing and transforming) #1260

youknowriad · 2017-06-19T12:59:43Z

I've noticed several issues with the quote block:

We were dropping the p when parsing the quote content
We were serializing double ps
Transforming to text block was broken when we moved the text block to single p.

This PR adds a new parsing helper node which is similar to children but includes the wrapper node.

youknowriad · 2017-06-19T13:01:12Z

blocks/api/test/query.js

+			const html = '<blockquote><p>A delicious sundae dessert</p></blockquote>';
+			const match = parse( html, query.node() );
+
+			expect( wp.element.renderToString( match ) ).to.equal( `<body>${ html }</body>` );


@aduth The extra body here seemed weird to me. Is this something automatically added by the parse function or something?

Also in the test above, the children helper is returning the whole HTML while I expected it to return only the inner p.

The body comes from hpq assigning innerHTML of a new document on which to perform the querying:

https://github.com/aduth/hpq/blob/a5bda80/src/index.js#L22

I seem to recall there may be alternatives to consider here to be able to avoid it.

Ok, doesn't bother me a lot now, but yeah maybe we should drop it somehow.

aduth · 2017-06-21T19:59:32Z

blocks/library/quote/index.js

@@ -59,9 +64,23 @@ registerBlockType( 'core/quote', {
 			{
 				type: 'block',
 				blocks: [ 'core/text' ],
-				transform: ( { value, citation } ) => {
+				transform: ( { value, citation, ...attrs } ) => {
+					const textElement = Array.isArray( value ) ? value[ 0 ] : value;


query should always return an array. When would the falsey flow occur here?

query no but IIRC it can occur in the Editable component if you have a single paragraph. I'll try to validate this tomorrow.

You were right, it seems like the ternary is not necessary here. I only needed a defaultValue for newly inserted quotes.

aduth · 2017-06-21T20:03:03Z

blocks/library/quote/index.js


 registerBlockType( 'core/quote', {
 	title: wp.i18n.__( 'Quote' ),
 	icon: 'format-quote',
 	category: 'common',

 	attributes: {
-		value: query( 'blockquote > p', children() ),
+		value: query( 'blockquote > p', node() ),


~~Would it be enough to change this to children( 'p' ) ?~~

Edit: Hmm, no, maybe not.

ellatrix · 2017-06-22T13:13:17Z

blocks/api/query.js

@@ -36,11 +36,11 @@ export const html = withKnownMatcherFlag( originalHtml );
 export const text = withKnownMatcherFlag( originalText );
 export const query = withKnownMatcherFlag( originalQuery );
 export const children = withKnownMatcherFlag( ( selector ) => {
-	return ( node ) => {
-		let match = node;
+	return ( domNode ) => {


Why rename?

to avoid the lint errors with the newly created node helper.

ellatrix · 2017-06-22T13:14:47Z

blocks/api/query.js

+			match = domNode.querySelector( selector );
+		}
+
+		return nodeToReact( match, wp.element.createElement );


I guess we don't want wp.element.createElement anymore?

ellatrix · 2017-06-22T13:20:17Z

blocks/test/fixtures/core-quote-style-1.json

-                [
-                    "The editor will endeavour to create a new page and post building experience that makes writing rich posts effortless, and has “blocks” to make it easy what today might take shortcodes, custom HTML, or “mystery meat” embed discovery."
-                ]
+                {


Could we do a test with multiple children?

ellatrix · 2017-06-22T13:24:13Z

I'm having some issues going from a quote => heading. Also, isn't it strange to do such a transformation?

Splits in two ^

youknowriad · 2017-06-22T13:29:05Z

@iseulde The transform to text also splits in two if there are multiple p in the quote block. Yes these are strange

ellatrix · 2017-06-22T13:42:47Z

So this is not a bug? I don't understand why it splits right in the middle of the text.

youknowriad · 2017-06-22T13:43:45Z

@iseulde The first paragraph is transformed to a heading while the others stay in the quote.

ellatrix · 2017-06-22T13:45:04Z

No, it's not the first one. There was only one.

ellatrix · 2017-06-22T13:46:02Z

It's splitting right before the italicised text.

youknowriad · 2017-06-22T13:47:37Z

Oh ok, that's a bug.

youknowriad · 2017-06-23T13:26:18Z

@iseulde MM I can't seem to reproduce the bug here.

ellatrix · 2017-06-23T13:28:16Z

@youknowriad Go to the "The Inserter Tool" heading in the demo, switch to quote, then back to heading.

youknowriad · 2017-06-26T08:58:53Z

@iseulde I can reproduce now Thanks.

I accidentally pushed the fix for this bug in master. Rebased here so this should be ok 599970e

youknowriad · 2017-06-27T10:56:57Z

@iseulde Going to merge this if no concerns?

ellatrix · 2017-06-27T11:01:08Z

Hm, now quote ==> heading no longer works.

youknowriad · 2017-06-27T11:09:14Z

Fixed, Let's hope this one is the good one.

ellatrix · 2017-06-27T11:14:40Z

blocks/library/heading/index.js

-						const headingContent = isString( content[ 0 ] )
-							? content[ 0 ]
-							: content[ 0 ].props.children;
+						const headingContent = isObject( content[ 0 ] ) && content[ 0 ].type === 'p'


I guess it doesn't matter much here, but not sure why we should change it, the string check being faster.

ellatrix · 2017-06-27T11:16:42Z

blocks/library/quote/index.js

@@ -63,9 +89,13 @@ registerBlockType( 'core/quote', {
 				blocks: [ 'core/heading' ],
 				transform: ( { value, citation, ...attrs } ) => {
 					const isMultiParagraph = Array.isArray( value ) && isObject( value[ 0 ] ) && value[ 0 ].type === 'p';
+					const headingElement = isMultiParagraph ? value[ 0 ] : value;
+					const headingContent = isObject( headingElement ) && value[ 0 ].type === 'p'


Similarly here, sometimes we seem to check for string, other times object. Nitpicking now though. :D

ellatrix · 2017-06-27T11:17:45Z

Good to merge once tests pass.

youknowriad added [Feature] Blocks Overall functionality of blocks [Type] Bug An existing feature does not function as intended labels Jun 19, 2017

youknowriad self-assigned this Jun 19, 2017

youknowriad requested review from nb and aduth June 19, 2017 12:59

youknowriad commented Jun 19, 2017

View reviewed changes

youknowriad mentioned this pull request Jun 19, 2017

Quote: Content not being output #1277

Closed

youknowriad force-pushed the fix/quote-parse-serialize branch from 66ec2e7 to 75c9f17 Compare June 21, 2017 10:46

aduth reviewed Jun 21, 2017

View reviewed changes

youknowriad force-pushed the fix/quote-parse-serialize branch from 9212460 to 3a7a696 Compare June 22, 2017 10:49

ellatrix reviewed Jun 22, 2017

View reviewed changes

youknowriad force-pushed the fix/quote-parse-serialize branch from 3a7a696 to 50abb9b Compare June 22, 2017 13:18

ellatrix reviewed Jun 22, 2017

View reviewed changes

youknowriad added 4 commits June 26, 2017 09:57

Blocks: Fix quote block (parsing, serializing and transforming)

7144d3e

Quote Block: Fix transforming an empty quote block to text

5cdee57

Quote Block: Adding a default value to avoid undefined content

6cce2c2

Use imports instead of the global wp variable

b05fc9f

youknowriad force-pushed the fix/quote-parse-serialize branch from 50abb9b to b05fc9f Compare June 26, 2017 08:58

ellatrix reviewed Jun 27, 2017

View reviewed changes

ellatrix approved these changes Jun 27, 2017

View reviewed changes

Quote Block: Fix single p transformations

fdd76b2

youknowriad force-pushed the fix/quote-parse-serialize branch from 4d8a843 to fdd76b2 Compare June 27, 2017 11:29

youknowriad merged commit f40484e into master Jun 27, 2017

youknowriad deleted the fix/quote-parse-serialize branch June 27, 2017 11:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Blocks: Fix quote block (parsing, serializing and transforming) #1260

Blocks: Fix quote block (parsing, serializing and transforming) #1260

youknowriad commented Jun 19, 2017 •

edited

Loading

youknowriad Jun 19, 2017

aduth Jun 19, 2017

youknowriad Jun 19, 2017

aduth Jun 21, 2017

youknowriad Jun 21, 2017 •

edited

Loading

youknowriad Jun 22, 2017

aduth Jun 21, 2017 •

edited

Loading

ellatrix Jun 22, 2017

youknowriad Jun 22, 2017

ellatrix Jun 22, 2017

youknowriad Jun 22, 2017

ellatrix Jun 22, 2017

ellatrix commented Jun 22, 2017 •

edited

Loading

youknowriad commented Jun 22, 2017

ellatrix commented Jun 22, 2017

youknowriad commented Jun 22, 2017

ellatrix commented Jun 22, 2017

ellatrix commented Jun 22, 2017

youknowriad commented Jun 22, 2017

youknowriad commented Jun 23, 2017

ellatrix commented Jun 23, 2017

youknowriad commented Jun 26, 2017

youknowriad commented Jun 27, 2017

ellatrix commented Jun 27, 2017

youknowriad commented Jun 27, 2017

ellatrix Jun 27, 2017

ellatrix Jun 27, 2017

ellatrix commented Jun 27, 2017

Blocks: Fix quote block (parsing, serializing and transforming) #1260

Blocks: Fix quote block (parsing, serializing and transforming) #1260

Conversation

youknowriad commented Jun 19, 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

youknowriad Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth Jun 21, 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

ellatrix commented Jun 22, 2017 • edited Loading

youknowriad commented Jun 22, 2017

ellatrix commented Jun 22, 2017

youknowriad commented Jun 22, 2017

ellatrix commented Jun 22, 2017

ellatrix commented Jun 22, 2017

youknowriad commented Jun 22, 2017

youknowriad commented Jun 23, 2017

ellatrix commented Jun 23, 2017

youknowriad commented Jun 26, 2017

youknowriad commented Jun 27, 2017

ellatrix commented Jun 27, 2017

youknowriad commented Jun 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellatrix commented Jun 27, 2017

youknowriad commented Jun 19, 2017 •

edited

Loading

youknowriad Jun 21, 2017 •

edited

Loading

aduth Jun 21, 2017 •

edited

Loading

ellatrix commented Jun 22, 2017 •

edited

Loading