-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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.
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
Quote Block: Fix serializing the quote block #583
Conversation
Can confirm that it fixes the problem for me! 👍 |
const { value, citation, style = 1 } = attributes; | ||
|
||
return ( | ||
<blockquote className={ `blocks-quote-style-${ style }` }> | ||
{ value && value.map( ( paragraph, i ) => ( | ||
{ value && wp.element.Children.map( value, ( paragraph, i ) => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the value
attributes definition, we should be certain that value
is an array, not one of React's opaque children structure:
query( 'blockquote > p', children() )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not when we insert an empty quote block, if we type inside the editable it'll not produce an array if we have a single paragraph.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not when we insert an empty quote block, if we type inside the editable it'll not produce an array if we have a single paragraph.
That's true, but in that case we have a bit of a mismatch between the shape of state on parse, and the value that will be assigned from Editable as described in your example.
Specifically, parse will treat a single paragraph as [ <p>foo</p> ]
, whereas in a new block, the same content would be assigned as just <p>foo</p>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really concerning? we could update the onChange
callback to match this if you prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, maybe not. Definitely want to avoid a situation where a developer would need to account for varying shapes of the value, if it ever comes to that (Array.isArray
checks, React.Children.map
when we know it's just one element, etc).
closes #564
Follow the steps in #564 and ensures that the block is not empty anymore.