Editor: Introduce name prop for RichText to handle changes automatically

[gziolo]

Description

Similar to #6419 where we introduced an automatic handling of focus for RichText component.

This PR proposes to make value and onChange props from RichText optional. When omitting those props you would have to use name instead. It would have to match the name of one of the block's attributes and it would enable the default handling of value and onChange props controlled by the editor.

+	edit( { className } ) {
-	edit( { className, attributes, setAttributes } ) {
		return (
			<RichText
				tagName="h2"
+				name="content"
				className={ className }
-				value={ attributes.content }
-				onChange={ ( content ) => setAttributes( { content } ) }
			/>
		);
	},

TODO

When we agree on the proposed improvement, we can also do:

• Apply the same changes to PlainText and update all usage in core blocks.
• Update all blocks that use RichText in a way that Paragraph block does.
• Update all docs to promote new approach whenever applicable.

In theory, it could be sprinkled on all controls, but we would have to fix the issue with the context not being properly applied to components rendered as children of fills. This applies to BlockControls, InspectorControls and AdvancedInspectorControls.

How has this been tested?

Manually.
I made sure that editing RichText still works as before when it's handled automatically with name props (Paragraph block) or with value and onChange for all other blocks.

Types of changes

Refactoring - improvement for developers convenience.

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.

[jorgefilipecosta]

We always used the setAttributes "concept", maybe here we can expose setAttributes, so we are not exposing a new concept (setAttribute).

[gziolo]

I don't have any preference here, I can update to setAttributes if it makes things easier.

[jorgefilipecosta]

Maybe we can leave this sample in the docs. As we still support using value + onChange and it may be useful in some cases.

[gziolo]

It's hard to tell, maybe we should update only Gutenberg handbook to contain the shorter version.

[jorgefilipecosta]

I feel our RichText component is getting bigger with more props. Maybe we can divide RichText into a more generic low-level version that does not use context. And a higher level version that uses context. When passing name the higher level version would just pass value + onChange to the low-level version. I feel this might simplify things and make it easier to add more high-level abstractions in the future.

[gziolo]

Yes, that's a good observation. It's definitely something we should explore soon to make sure RichText doesn't reach 1k lines :)

[zackify]

@jorgefilipecosta I agree with you, it's really hard to make sure there's no bugs with how big it's getting. A better idea in my opinion would be to compose components and add features that way.

[jorgefilipecosta]

I like this new name prop, and I think it is a good step to make block creation simpler 👍
Maybe we can name the prop to attributeName, just to make it clear that name refers to the attribute. But given that we already have tagName and className it would seem all the props end in "name" (which is not very useful). So another option is call the prop "attribute" and rename the prop "tagName" to "tag" (essentially removing the "name" suffix).

[gziolo]

attributeName - I thought about that, too. I can update to attribute or attributeName - both work for me.

[aduth]

Apply the same changes to PlainText and update all usage in core blocks.

What about AlignmentToolbar, FontSizePicker, ToggleControl, PanelColor, TextareaControl, SelectControl, FormFileUpload, UrlInput, RangeControl, CodeEditor ?

How far do we take it? If a block implementer comes to expect this to be automated from RichText and PlainText, will they be caught off guard when it's exposed to them through other inputs?

I mean, I like the idea, and it could encourage developers to use the abstracted component forms rather than the DOM primitives (e.g. input), but only if applied consistently.

I think also @mtias mentioned this at one point in the past as being a good thing to offer.

[gziolo]

What about AlignmentToolbar, FontSizePicker, ToggleControl, PanelColor, TextareaControl, SelectControl, FormFileUpload, UrlInput, RangeControl, CodeEditor ?

How far do we take it? If a block implementer comes to expect this to be automated from RichText and PlainText, will they be caught off guard when it's exposed to them through other inputs?

See my note in the description:

In theory, it could be sprinkled on all controls, but we would have to fix the issue with the context not being properly applied to components rendered as children of fills. This applies to BlockControls, InspectorControls and AdvancedInspectorControls.

As usual in such case, my answer is - yes for all for consistency. Technically it is possible, but it would require forwarding the context. There is also an architectural decision to be made, because by design all components in components module should be independent of other modules. So the idea would be to have wrapper components in editor module as explained by @youknowriad in our chat on Slack:

We should probably normalize all these components to expect a similar API (value, and onChange) and have an HoC providing these props if name is defined

[mtias]

I mean, I like the idea, and it could encourage developers to use the abstracted component forms rather than the DOM primitives (e.g. input), but only if applied consistently.

Yes, this would be key and one of the reasons we didn't push for it earlier — once we go with the abstraction it has to be the right one, as we are bound to support it.

I agree with going with attribute or attributeName instead of name as the relationship with attributes has to be very clear.

@nb had expressed wanting something like this when building blocks, so looping him in.

The UI control components are trickier in that we could be generating excessive noise by wrapping them with others, and having to explain when you use which. It doesn't sound like a great situation to be in.

[gziolo]

Just noting that a very similar idea was also proposed by @zackify in gutenblock plugin but using another wrapper component:

import { RichText } from 'gutenblock-controls';

const Edit = () => (
  <div>
    <RichText name="description" />
  </div>
);

I will reach out to him to collect feedback on this one.

[aduth]

See my note in the description:

In theory, it could be sprinkled on all controls, but we would have to fix the issue with the context not being properly applied to components rendered as children of fills. This applies to BlockControls, InspectorControls and AdvancedInspectorControls.

As usual in such case, my answer is - yes for all for consistency. Technically it is possible, but it would require forwarding the context. There is also an architectural decision to be made, because by design all components in components module should be independent of other modules. So the idea would be to have wrapper components in editor module as explained by @youknowriad in our chat on Slack

Another reading comprehension fail on my part 😅

I think I've mentioned on at least one earlier occasion, but I'd love if we could migrate toward an exclusively portal-based slot/fill, since I believe it should only be the one based in DOM that is problematic.

Some prior art for context forwarding, though requires explicitly targeting values: https://github.com/Automattic/wp-calypso/blob/44ca12f43a99dde4e65a00bd0febf5d143fad323/client/components/root-child/index.jsx#L46-L50

[gziolo]

Closing it for now as it got outdated. We should definitely get back to this idea after WordPress 5.0 is out.