Parsing: Declare all block attributes #714

youknowriad · 2017-05-08T16:36:48Z

closes #609

This PR tries to resolve Question: How can we surface expected attributes of a block? #609 by allowing a chainable API to define blocks.
With this PR, all block attributes should be defined in the attributes property
The attributes property` can not be defined as a function anymore
Adds the metadata descriptor to declare an attribute as a comment attribute.

This PR doesn't solve the typing yet, but the chainable API allows easily to add type descriptors (isString, isNumber) etc...

The chainable API allows things like that #609 (comment)

mtias · 2017-05-08T18:09:48Z

blocks/api/parser.js

-			...parseBlockAttributes( rawContent, blockSettings ),
-		};
-	}
+	const computedAttributes = reduce( blockSettings.attributes, ( memo, attribute, key ) => {


Can we explain a bit more how this works here and the motivation behind it?

aduth · 2017-05-09T14:32:18Z

blocks/library/button/index.js

@@ -32,7 +32,8 @@ registerBlock( 'core/button', {
 	attributes: {
 		url: attr( 'a', 'href' ),
 		title: attr( 'a', 'title' ),
-		text: children( 'a' )
+		text: children( 'a' ),
+		align: metadata()


I'm personally on the fence, but how do you feel about being even more explicit here with defining the metadata key to be extracted, rather than inferred by the key of the attribute.

For example:

align: metadata( 'align' )

My worry with this would be that there'd likely be very undesirable consequences if the developer were to make a typo or otherwise use an argument different from the key.

That was the first API I had, I made it optional in a second step. I thought it'd be handy to avoid duplication, but I'm ok reverting to a mandatory attribute name.

That was the first API I had, I made it optional in a second step. I thought it'd be handy to avoid duplication, but I'm ok reverting to a mandatory attribute name.

To avoid errors, it's probably better to keep as-is.

aduth · 2017-05-09T20:16:14Z

There's a side-question here of: is "metadata" the terminology we want to use for describing attributes serialized in the block comment?

aduth · 2017-05-09T20:29:03Z

I'm not feeling the chainable API. Do you think if we were able to encode more types like numbers in the serialized comment itself, we'd even need it?

youknowriad · 2017-05-09T20:50:44Z

I'm not feeling the chainable API. Do you think if we were able to encode more types like numbers in the serialized comment itself, we'd even need it?

I can understand. But the answer to the question is yes we need something to describe the arguments.

I think the important part of this PR is introducing the attributes "metadata" (or descriptors, or whatever, we should choose a name). The attribute metadata could contain:

The parsing information (what we currently have)
The type information (even if the comment is typed, we'd need this information for the inspector)
A description of the attribute (could be used in the inspector)
A flag saying whether the attribute should be shown in the inspector or not
And maybe any other information required to "generate" an attribute form. (Choices for example) or even a fallback to a generic form which could be a React Element or something.

Now, how does a block author describe these metadata. I tought the chaining API could be a sugar syntax for this, but It's not a "requirement" for me. Having a more explicit object descriptor like this is ok for me to:

attributes: {
  align: {
    source: metadata(),
    type: 'string',
    choices: [ 'left', 'right' ],
    description: 'Text Align'
    showInInspector: true
  }
}

aduth · 2017-05-09T21:28:34Z

I'd separately commented at #731 (comment) that I'm not sure I see reason that we need force inspector to be merged into attributes. Even if we had form builder logic included, would it be much worse as:

attributes: {
	align: metadata()
},

inspector: [
	{
		attribute: 'align',
		type: 'string',
		choices: [ /* ... */ ],
		description: '...'
	}
]

Note also that this accounts for field ordering.

I also don't have a good sense of the sorts of fields we'll expect to be included in an inspector. Should we really expect them to map so cleanly to attributes? To only display fields of supported types (dropdowns, text inputs)? What about custom inputs?

All this being said, I think I could be convinced that it's nice to treat attributes as a single source for type, choices, description, etc.

youknowriad · 2017-05-10T08:43:13Z

I also don't have a good sense of the sorts of fields we'll expect to be included in an inspector. Should we really expect them to map so cleanly to attributes? To only display fields of supported types (dropdowns, text inputs)? What about custom inputs?

Me neither but I think we could display all attributes except "content" attributes. It's not very clear in my suggestion above but I'm proposing a form fallback as a React component for custom fields.

All this being said, I think I could be convinced that it's nice to treat attributes as a single source for type, choices, description, etc.

Yeah, I don't see much reasons to split the parsing information of attributes from the other descriptors which are generic information about the attribute and not specifically tied to the inspector. They serve for attributes "reflexion". They could be useful elsewhere.

youknowriad · 2017-05-11T08:35:16Z

How can we move this forward? Should we take a decision now, or maybe I can update the PR to leave the metadata function but drop the chainable API?

jasmussen · 2017-05-11T09:40:06Z

How can we move this forward? Should we take a decision now, or maybe I can update the PR to leave the metadata function but drop the chainable API?

I think ultimately the decision we make here should be made by those of you with technical prowess. But I'd like to give my high level thoughts on the purpose of the inspector, what problems it hopes to solve, and perhaps that can help guide a technical decision.

Traditionally WordPress on the desktop has a quick toolbar with access to bold, italic, etc. Some actions, like editing a gallery or an audio shortcode, or a video shortcode, these all open modal windows:

On mobile, however, these actions are rarely surfaced at all:

The modal is also disruptive to the flow, and doesn't show context, as it essentially "covers the screen".

The inspector is an attempt at fixing that, across mobile and desktop, by ensuring that all metadata lives in a single place, the Post Settings sidebar. On desktop, the post settings sidebar is shown by default, but can be toggled off. On mobile, the post settings sidebar has to be toggled when you want to access it.

We are essentially levelling the playing field: all metadata lives in Post Settings. With no blocks selected you see the document level metadata — tags, categories, etc. When a block is selected, you see the block level metadata.

Block level metadata is:

basic items in the quick toolbar — bold, italic, alignments, etc (blueprints reminder)
advanced items in the inspector — dropcap for text, alt text for images, and anything that we would have previously put inside a modal

The decision on how to proceed should probably be informed by looking at what plugin authors currently put inside modals.

Here are some more modal screenshots:

Note: In the name of shipping I don't expect us to replace all those modals in V1 at all — that'll likely take time. But it would be the ultimate dream goal, to not need modals at all.

aduth · 2017-05-12T15:30:12Z

Sorry I've let this one linger. Not specifically to the pieces addressed here, but matcher APIs have been on my mind lately related to #608 (still very easy to gain access to DOM properties, lacking some tools for deduplication) and #771 (better representation of children data). As such, I'm a bit hesitant to move forward with further expanding upon what we have. Is this one blocking you?

youknowriad · 2017-05-12T15:36:58Z

As such, I'm a bit hesitant to move forward with further expanding upon what we have. Is this one blocking you?

Fair enough, I was planning to reuse some of these to start experimenting with the inspector, but it's not really blocking, let's close it for now. Fine to revisit later

aduth · 2017-05-24T19:19:58Z

Related: #672

Parsing: Declare all block attributes

1124cfe

youknowriad added the [Feature] Parsing Related to efforts to improving the parsing of a string of data and converting it into a different f label May 8, 2017

youknowriad self-assigned this May 8, 2017

youknowriad requested a review from aduth May 8, 2017 16:36

mtias reviewed May 8, 2017

View reviewed changes

Parser: Adds comments to attributes parsing

1800bba

youknowriad force-pushed the update/attributes-parsing branch from 15fed13 to 1800bba Compare May 9, 2017 09:14

youknowriad mentioned this pull request May 9, 2017

Add Slot to render block settings in the inspector. #731

Merged

aduth reviewed May 9, 2017

View reviewed changes

youknowriad closed this May 12, 2017

aduth mentioned this pull request May 22, 2017

Discussion: Block attributes initialization #865

Closed

aduth mentioned this pull request May 24, 2017

Try: Alternative attributes initialization #892

Closed

aduth mentioned this pull request Jun 12, 2017

Gallery: Add simple columns slider #1135

Merged

aduth mentioned this pull request Jul 14, 2017

Enhance attributes to specify types and defaults #1905

Merged

ntwb deleted the update/attributes-parsing branch August 5, 2017 01:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parsing: Declare all block attributes #714

Parsing: Declare all block attributes #714

youknowriad commented May 8, 2017

mtias May 8, 2017

aduth May 9, 2017

youknowriad May 9, 2017

aduth May 9, 2017

aduth commented May 9, 2017 •

edited

Loading

aduth commented May 9, 2017

youknowriad commented May 9, 2017

aduth commented May 9, 2017

youknowriad commented May 10, 2017

youknowriad commented May 11, 2017

jasmussen commented May 11, 2017 •

edited

Loading

aduth commented May 12, 2017 •

edited

Loading

youknowriad commented May 12, 2017

aduth commented May 24, 2017

Parsing: Declare all block attributes #714

Parsing: Declare all block attributes #714

Conversation

youknowriad commented May 8, 2017

mtias May 8, 2017

Choose a reason for hiding this comment

aduth May 9, 2017

Choose a reason for hiding this comment

youknowriad May 9, 2017

Choose a reason for hiding this comment

aduth May 9, 2017

Choose a reason for hiding this comment

aduth commented May 9, 2017 • edited Loading

aduth commented May 9, 2017

youknowriad commented May 9, 2017

aduth commented May 9, 2017

youknowriad commented May 10, 2017

youknowriad commented May 11, 2017

jasmussen commented May 11, 2017 • edited Loading

aduth commented May 12, 2017 • edited Loading

youknowriad commented May 12, 2017

aduth commented May 24, 2017

aduth commented May 9, 2017 •

edited

Loading

jasmussen commented May 11, 2017 •

edited

Loading

aduth commented May 12, 2017 •

edited

Loading