Gutenberg: Improve block settings and its methods

[mtias]

settings.edit and settings.save, specially in how they are used in the visual editor block, is confusing to read: are these actions? Do they trigger actions? What would happen when they are called?

We should be specially clear on what these are. I'd expect to call them via block.forEditing, perhaps. The presence of a settings object that is retrieved through wp.blocks.getBlockSettings( block.blockType ) doesn't help, but since it is sufficiently private it might be alright. Since edit/save in their current form are descriptions of how these blocks manifest or are represented in different contexts, renaming settings to something else seems like a good thing.

[ellatrix]

Also regarding the block settings: I've been thinking a bit about the way we're declaring controls. Currently it looks something like this:

{
    icon: 'editor-alignright',
    title: wp.i18n.__( 'Align right' ),
    isActive: ( { align } ) => 'right' === align,
    onClick( attributes, setAttributes ) {
        setAttributes( { align: 'right' } );
    }
}

Which is quite verbose. Maybe it makes sense to just specify the new attribute(s) the block needs to have when this control is clicked? To see if it's active we'd just look at the current attributes.

{
    icon: 'editor-alignright',
    title: wp.i18n.__( 'Align right' ),
    attributes: { align: 'right' }
}

Any concerns?

[aduth]

Maybe it makes sense to just specify the new attribute(s) the block needs to have when this control is clicked?

My first reaction was that this would only work well for the very simple cases. The more I'm looking through mockups though, the less I'm able to find cases where blocks need more granular control. One subtle nuance of this behavior worth noting is that if the attribute value is already the current value, clicking the button again effectively resets it to a default state. How do we infer what the default state would be? For text it's nothing, for an image it's explicitly the "none" alignment, for a heading it's the h2 tag, for a quote it's quote-style-1, etc. It's enough to infer undefined value of the attribute from the edit and save functions, but we'd still likely want to show the appropriate active state of the button in the toolbar.

Also, will a control need to ever do more than toggle an attribute of a block? I guess I'd assumed yes. Perhaps something like displaying a modal which could affect the block by asynchronously a deferred call to the setAttributes argument? Maybe it needs to append something to its own attributes? Maybe it needs to insert a new block before or after? These are hypotheticals but worth considering.

Maybe this syntax is still supported but effectively treated as a shorthand to the underlying equivalents achieved in the current API.

Aside: I think we ought to create a separate issue for this discussion.

[aduth]

confusing to read: are these actions? Do they trigger actions? What would happen when they are called?

Ignoring preexisting knowledge, what would one expect to be the result of calling a React component's render method? Seems quite similar as far as verb-to-action assumptions go.

I'm not terribly attached to the naming though. forEditing and forSave at least more accurately illustrate that the return value is a mere description of the UI. I guess I'm just averse to syllables 😄

[ellatrix]

One subtle nuance of this behavior worth noting is that if the attribute value is already the current value, clicking the button again effectively resets it to a default state.

I've thought about this too, but are there really any buttons that need to do two different actions (toggle)? Should clicking an active button render it to the default attribute, or should there be a button for that? Looking at the mockups there seem to be explicit buttons for all of these examples.

I agree though that the current methods should still be supported for more advanced controls.

[aduth]

I've thought about this too, but are there really any buttons that need to do two different actions (toggle)?

This is how alignment controls work for text and images in the current editor and I assumed would be the same here. I don't have a strong preference one way or the other.

[joyously]

How do we infer what the default state would be? For text it's nothing, for an image it's explicitly the "none" alignment, for a heading it's the h2 tag, for a quote it's quote-style-1, etc.

For an image it's nothing also, not "none". You don't want to be adding that class to all images if it wasn't there already. Empty is a valid state.

[aduth]

For an image it's nothing also, not "none". You don't want to be adding that class to all images if it wasn't there already. Empty is a valid state.

Depends if we're talking about what the current editor does now, how we want to represent the none state in a block, or what the new editor will be expected to output.

• The current editor does add an explicit alignnone class for unaligned images.
• I don't feel strongly one way or the other about how a block internally represents the unaligned image. A "none" value would be fine.
• Do we need to be concerned about backwards compatibility for themes which may have applied styles to the alignnone class? If so, we may still want to assign this to images output in the new editor.

There's also the case of how to handle images which may have been manually inserted or modified to omit the alignnone class. This is more to a general question of: If the editor can detect legacy content and associate it with a known block type, is it expected that the next save will regenerate the markup according to the block's own logic, or should it not touch the original content? There are compromises that need to be made to both answers of this question.

[joyously]

is it expected that the next save will regenerate the markup according to the block's own logic, or should it not touch the original content?

Since the editor has two parts, Visual and Text Mode, it is optional to use the Visual part. So that should not affect the original content. If I switch back and forth, will it mangle my content? The current editor has been known to do that. That's one of the reasons I stay out of it.
It's conceivable to me that themes could style alignnone to be float:none. Adding that class to all images that didn't have an alignment could make a mess out of existing content.

[BE-Webdesign]

Possibly name edit() => editView(), and save() => view(). The output is a view component for both methods. forEditing(), to me, is just as vague as edit() in some ways, but overall forEditing() is probably better than just edit().

[aduth]

Yeah, something about the "for" feels off for me. I'm wondering if we're trying too hard to preserve conciseness. Maybe an intentionally verbose alternative could lend some clarity.

A plethora of combination ideas from the top of my head:

(get|to|as) + (Editor|Save) + (View|Rendered|Content|Element)

[BE-Webdesign]

(get|to|as) + (Editor|Save) + (View|Rendered|Content|Element)

LOL any combination of these would be fine (funny how that works). Maybe asEditorView, asRenderedView would be the most accurate? Or maybe the term View is not widely used and Element|Component could replace View?

[aduth]

Yeah I don't think "View" has as much meaning in a React context. "Component" is not quite accurate since the return value of the function is an element, of whose type can be a component.

https://facebook.github.io/react/blog/2015/12/18/react-components-elements-and-instances.html

[BE-Webdesign]

https://facebook.github.io/react/blog/2015/12/18/react-components-elements-and-instances.html

Gratzi.

[mtias]

@aduth @nb @ehg if we want to get this changed, we should do so sooner rather than later.

[aduth]

"Component" is not quite accurate since the return value of the function is an element

Clarifying this further, edit and save can themselves be more accurately described as components. In many cases they are function components. getEditorElement would not be a great name, as if it were defined as a component value, it would read awkwardly:

getEditorElement: class extends Component {

The current names haven't bothered me quite as much, or at least I haven't seen a better name in the context of registering the block's settings. In hindsight, even forEditing is odd because in relation to other block settings, it lacks context to know what one should expect the value to contain.

Another idea that comes to mind is nesting variants under an object property, like components, render, or context:

```js
registerBlockSettings( 'core/text', {
components: {
editor: function() {},
save: function() {}
}
} );

[ehg]

Another idea that comes to mind is nesting variants under an object property, like components, render, or context:

I quite like this :)

[youknowriad]

Honestly, I'm finding the current edit and save as the best options proposed here. I'd vote for keeping them.

the components wrapper? maybe, but this could confuse people in thinking they'd have to use the Component class and IMO we should forbid the component class for the save function.