Block API: add more inline comments #20257
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mtias
added
[Feature] Block API
mtias
requested review from
ajitbohra,
chrisvanpatten,
daniloercoli,
ellatrix,
etoledom,
jorgefilipecosta,
mkaz,
mkevins,
pinarol,
SergioEstevao,
Soean,
talldan and
youknowriad
as code owners
mtias
force-pushed
the
update/block-api-inline-comments
branch
3 times, most recently
from
fbde271 to
e704ff2
Compare
mcsf
reviewed
Feb 17, 2020
There was a problem hiding this comment.
I definitely appreciate this voice, conducive to broad explanations and the offering of a starting point to understanding the blocks API.
However, it does raise interesting question about how and where to accommodate this kind of documentation. In its current form, the language added to ./index.js may prove immensely useful to one stumbling upon the file, but there's a serendipitous element to that; other devs may instead look for information in ./README.md (non-existent) or ../../README.md (too prosaic), for a very general view, or ./parser/README.md, ./serializer/README.md for each topic.
Clearly, there are requirements of discoverability, locality and depth to juggle. Avoiding duplication is also important if documentation is to remain current. Can we find a better balance? I don't know if the bulk of the comments in this PR should make up a new packages/blocks/src/api/README.md, or if they should be broken up by topic, i.e. spread out in ./parser/README.md, ./serializer/README.md, etc.
Either way, one file should point to the other(s). As an illustration:
// Serialization is covered extensively in .././path/
export { … } from './serializer';
packages/blocks/src/api/parser.js
Outdated
| if ( ! deprecatedDefinitions || ! deprecatedDefinitions.length ) { | ||
| return block; | ||
| } | ||
|
|
||
| const { originalContent, innerBlocks } = block; | ||
|
|
||
| // By design, blocks lack any sort of version tracking. Instead, to process | ||
| // outdated content it operates a queue out of all the defined attribute |
packages/blocks/src/api/index.js
Outdated
| @@ -1,3 +1,9 @@ | |||
| // The Block Type is the most important concept within the block API. It defines | |||
| // all aspects of the block configuration and its interfaces (including "edit" | |||
| // and "save"). The transforms specification allows converting one block type to | |||
There was a problem hiding this comment.
For consistency with other occurrences, edit and save should be wrapped with back-ticks instead.
packages/blocks/src/api/index.js
Outdated
| // blocks mechanism. A child block is a block node that can only exist within | ||
| // the inner block boundaries of a specific parent. This allows block authors to | ||
| // compose specific block nodes that are not meant to be used outside of a | ||
| // specific context. Thus, child blocks extend the concept of inner blocks to |
There was a problem hiding this comment.
The description in terms of block nodes rather than types, though pertinent, may mislead one into understanding the child relationship to be instance-based (dynamically set on the node) rather than class-based (defined in the type).
mcsf
reviewed
Feb 17, 2020
packages/blocks/src/api/parser.js
Outdated
| @@ -617,7 +634,20 @@ const createParse = ( parseImplementation ) => ( content ) => | |||
| }, [] ); | |||
|
|
|||
| /** | |||
| * Parses the post content with a PegJS grammar and returns a list of blocks. | |||
| Utilizes an optimized token-driven parser based on the Gutenberg grammar spec | |||
There was a problem hiding this comment.
Something went wrong with the automated suggestion thing: needs a leading *.
packages/blocks/src/api/parser.js
Outdated
| @@ -404,8 +412,13 @@ export function getMigratedBlock( block, parsedAttributes ) { | |||
| */ | |||
| export function createBlockWithFallback( blockNode ) { | |||
| const { blockName: originalName } = blockNode; | |||
| // The tripartite structure of the block type includes its attributes, inner | |||
There was a problem hiding this comment.
"Tripartite" is a very obscure word; perhaps this should be simplified to "The structure of a block type includes its attributes, inner blocks, and inner HTML."
packages/blocks/src/api/index.js
Outdated
| @@ -1,3 +1,9 @@ | |||
| // The Block Type is the most important concept within the block API. It defines | |||
There was a problem hiding this comment.
Should "Block Type" be capitalized or not? It isn't elsewhere in these comments.
gziolo
added
[Status] Stale
|
Any plans to address feedback? Do you need help with this one? |
|
I need time |
|
|
A Dali reference in git hub repo 💕🥰 |
mtias
force-pushed
the
update/block-api-inline-comments
branch
2 times, most recently
from
2fcdddb to
734b142
Compare
|
Size Change: 0 B Total Size: 1.28 MB ℹ️ View Unchanged
|
Co-Authored-By: Miguel Fonseca <[email protected]>
mtias
force-pushed
the
update/block-api-inline-comments
branch
from
b97b226 to
b96c1c6
Compare
|
I found time! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When reading the source code for the block API there's a patent lack of overview around some key concepts and their relationships that I feel is worth clarifying next to the code.
It also expands a bit more on the role of the PEG parser.