RichText: Add the RichText.Content component to be used in conjunction with RichText #6274

youknowriad · 2018-04-19T09:43:21Z

related #6034

This PR adds a RichText.Content to clarify the usage of RichText values in the save function. Block authors don't need to know that the value is a react element and can be embedded. They just declare that it's a richtext value with a given format:

<RichText.Content>{ value }</RichText.Content>

Questions

Right now the API of RichText.Content doesn't match directly the API of RichText so we end up with something like this:

<RichText tagName="h2" value={ attributes.content ) /> // This is the edit function 

<h2><RichText.Content>{attributes.content}</RichText.Content></h2> // This is the save function

I was thinking that we could align both which means:

Adding tagName prop to RichText.Content
Use value prop instead of children
Support multiple (if multiple is not false, don't render any parent tag name)

Thoughts?

Notes

One of the ideas behind this component is that it will allow us to switch the default format at some point (tree or string) with a clear deprecation strategy.

aduth

I went with value prop in #5380 . One advantage is that it doesn't mislead one to think there's a wrapper element applied, which at a glance it might appear there is.

aduth · 2018-04-19T15:04:29Z

blocks/rich-text/README.md

@@ -95,6 +100,12 @@ wp.blocks.registerBlockType( /* ... */, {
 			}
 		} );
 	},
+
+	save: function() {
+		return wp.element.createElement( 'h2, {},


Missing single quote after h2.

aduth · 2018-04-19T15:05:06Z

blocks/rich-text/index.js

@@ -876,7 +876,7 @@ RichText.defaultProps = {
 	format: 'element',
 };

-export default compose( [
+const RichTextContainer = compose( [


Can we just reassign RichText here? RichText = compose( [ ] )( RichText );

This was not possible because we use the inner RichText in unit tests.

aduth · 2018-04-19T15:06:29Z

docs/blocks-editable.md

@@ -112,4 +118,4 @@ The `RichText` component can be considered as a super-powered `textarea` element

 Implementing this behavior as a component enables you as the block implementer to be much more granular about editable fields. Your block may not need `RichText` at all, or it may need many independent `RichText` elements, each operating on a subset of the overall block state.

-Because `RichText` allows for nested nodes, you'll most often use it in conjunction with the `children` attribute source when extracting the value from saved content.
+Because `RichText` allows for nested nodes, you'll most often use it in conjunction with the `children` attribute source when extracting the value from saved content. You'll also most likely use `RichText.Content` in the `save` function to output RichText values.


"most likely" ? Uncertainty here would be confusing to a developer, particularly without explanation of if/when it should be applied.

I'm assuming this is because the quote values are left alone? Not sure. Will be simpler after #6054.

ellatrix · 2018-04-19T15:18:07Z

Support multiple (if multiple is not false, don't render any parent tag name)

What do you mean with this?

I lean towards the value prop as well.

Makes it very similar in use to RichText.
Perhaps we could also incorporate empty checking into it, so we return null instead of an empty <TagName> when value is empty. This could be very handy for e.g. <figcaption>.

youknowriad · 2018-04-19T16:52:50Z

I was thinking:

<RichText.Content tagName="h2" value={ "blabla" } /> => <h2>blabla</h2>

<RichText.Content multiline value={ ab } />(or multiline="p" to match the RichText) => ab

<RichText.content value={"blabla"} /> => blabla (default TagName)

This way it matches RichText entirely

ellatrix · 2018-04-19T17:14:29Z

Sounds great!

…n with RichText

youknowriad · 2018-04-20T09:24:33Z

Updated, I think it's nicer this way where we mimic the same API as RichText.

I was thinking of introducing a console.info message to encourage users to use RichText.Content what do you think? what should be the wording of the message?

aduth · 2018-04-20T13:30:11Z

I was thinking of introducing a console.info message to encourage users to use RichText.Content what do you think? what should be the wording of the message?

When would it be logged? There's a legitimate concern here that this is less convenient for developers in some way, as the previous approach was "simpler", albeit not always consistent with the new format options.

youknowriad · 2018-04-20T14:15:28Z

When would it be logged?

I was thinking to always log it once when you load Gutenberg (RichText file) for some versions. I wonder how we can communicate about this better?

ellatrix · 2018-04-20T16:20:14Z

blocks/library/video/index.js

@@ -194,7 +194,7 @@ export const settings = {

 			<figure className={ align ? `align${ align }` : null }>
 				{ src && <video controls src={ src } /> }
-				{ caption && caption.length > 0 && <figcaption>{ caption }</figcaption> }
+				{ caption && caption.length > 0 && <RichText.Content tagName="figcaption" value={ caption } /> }


Just wondering, but would it be good to incorporate the empty check in RichText.Content? This seems to be a common pattern. Even if the block registration has no such check, I don't think there are any cases where it makes sense to render an empty element in save context.

I'm on the fence here because I wonder if it's possible sometimes to render the container tag without its content ( like an empty list item or something). Let's keep it as is for now.

ellatrix · 2018-04-20T16:22:07Z

There's a legitimate concern here that this is less convenient for developers in some way, as the previous approach was "simpler", albeit not always consistent with the new format options.

It seems we all agree we need to move away form storing the React objects, so this will be beneficial in any case, unless you'd prefer to handle this in the save function? It does feel strange to have non React objects as children though.

I'm also not sure if I agree that this is more complicated... It's more verbose, yes, but it's simple in the sense that it's very similar to what you use for the edit function now.

ellatrix

I like it. +1 from me.

youknowriad · 2018-04-23T08:11:16Z

After a second thought, I'm going to merge this as is. The warning should be introduced later when we settle on the format we want as a replacement for the default "element" format. That way we will ask people to only update their code once and not twice (Add RichText.Content then switch the format)

…n with RichText (WordPress#6274) * RichText: Add the RichText.Content component to be used in conjunction with RichText * Support tagName and extra props in RichText.Content * Use RichText.Content in quotes as well * Fix unit tests

youknowriad requested review from aduth, ellatrix, gziolo, mcsf, mtias and jorgefilipecosta April 19, 2018 09:43

youknowriad self-assigned this Apr 19, 2018

aduth reviewed Apr 19, 2018

View reviewed changes

youknowriad added 3 commits April 20, 2018 10:21

RichText: Add the RichText.Content component to be used in conjunctio…

90852ca

…n with RichText

Support tagName and extra props in RichText.Content

f89908f

Use RichText.Content in quotes as well

9865287

youknowriad force-pushed the try/rich-text-content branch from bb3f651 to 9865287 Compare April 20, 2018 09:21

Fix unit tests

678e52e

ellatrix reviewed Apr 20, 2018

View reviewed changes

ellatrix approved these changes Apr 20, 2018

View reviewed changes

youknowriad merged commit 9213b9a into master Apr 23, 2018

youknowriad deleted the try/rich-text-content branch April 23, 2018 08:15

salcode mentioned this pull request May 1, 2018

Consider only Updating Documentation on Release #6514

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RichText: Add the RichText.Content component to be used in conjunction with RichText #6274

RichText: Add the RichText.Content component to be used in conjunction with RichText #6274

youknowriad commented Apr 19, 2018 •

edited

Loading

aduth left a comment

aduth Apr 19, 2018

aduth Apr 19, 2018

youknowriad Apr 20, 2018

aduth Apr 19, 2018

ellatrix Apr 19, 2018

ellatrix commented Apr 19, 2018

youknowriad commented Apr 19, 2018

ellatrix commented Apr 19, 2018

youknowriad commented Apr 20, 2018 •

edited

Loading

aduth commented Apr 20, 2018

youknowriad commented Apr 20, 2018

ellatrix Apr 20, 2018

youknowriad Apr 23, 2018

ellatrix commented Apr 20, 2018

ellatrix left a comment

youknowriad commented Apr 23, 2018

RichText: Add the RichText.Content component to be used in conjunction with RichText #6274

RichText: Add the RichText.Content component to be used in conjunction with RichText #6274

Conversation

youknowriad commented Apr 19, 2018 • edited Loading

aduth left a comment

Choose a reason for hiding this comment

aduth Apr 19, 2018

Choose a reason for hiding this comment

aduth Apr 19, 2018

Choose a reason for hiding this comment

youknowriad Apr 20, 2018

Choose a reason for hiding this comment

aduth Apr 19, 2018

Choose a reason for hiding this comment

ellatrix Apr 19, 2018

Choose a reason for hiding this comment

ellatrix commented Apr 19, 2018

youknowriad commented Apr 19, 2018

ellatrix commented Apr 19, 2018

youknowriad commented Apr 20, 2018 • edited Loading

aduth commented Apr 20, 2018

youknowriad commented Apr 20, 2018

ellatrix Apr 20, 2018

Choose a reason for hiding this comment

youknowriad Apr 23, 2018

Choose a reason for hiding this comment

ellatrix commented Apr 20, 2018

ellatrix left a comment

Choose a reason for hiding this comment

youknowriad commented Apr 23, 2018

youknowriad commented Apr 19, 2018 •

edited

Loading

youknowriad commented Apr 20, 2018 •

edited

Loading