-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add unimplemented block structuring and complementary modules #284
Conversation
Pulling comments out of commit into pull request in case I need to rebase: cc1cdb2#commitcomment-21359526
Correct.
The idea would be that it's the block content state, i.e. image's
This is a good point, though with some of your examples based on previous needs in the multi-instance approach where block implementation is responsible for toolbars, movement, focus state, some of which could be applied as styles cascaded from an ancestor element rendered by common block logic. An idea to consider would be nesting all of the options we'll need in an object, where destructuring allows us to easily pick and choose the parts we'll need: edit( { state, onChange } ) {
// Or: edit( { state } )
// Or: edit( { isFocused } )
// Or: edit( allEncompassingBlobOfBlockDetailsAndActionsForWhichWeNeedABetterName )
// etc. cc1cdb2#commitcomment-21359529
Similar to cc1cdb2#commitcomment-21358856
Is this in reference to the folder name? What about |
} ); | ||
}, | ||
save( state ) { | ||
return createElement( 'p', null, state.value ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you think it's worth it to use an element here instead of just a string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you think it's worth it to use an element here instead of just a string?
Depends whether we want to support multiple return types or not. In this case we assume the save
function must return of type "Element". String is perfectly suitable, but we'd need to build support for it and then question whether overloading the method return type is really benefiting us more than the cost of confusion around supporting multiple types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need multiple return types? String could work for all use-cases, right?
Why do we need it to be an element (which is HTML + events instead of just HTML)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need it to be an element (which is HTML + events instead of just HTML)?
It's important to point out that this is as simple as blocks get. For everything else, createElement
's purpose doubles as a convenient DOM builder.
While also true that an element could be stringified without too much trouble, (a) it's still nicer if you don't have to, and (b) I find the consistency with edit
leads to a pleasant duality.
Also, even if we've found in prototypes we'd needed additional features, I'm inclined to keep it simple to start, as some of the newer ideas around consolidating state and eliminating parsing and serialization behaviors from blocks may lead us to find they're not strictly necessary. The idea around passing an object which we can scale with additional properties as warranted is appealing, however. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good start of an API for me. Let's ship it 👍
@@ -0,0 +1,24 @@ | |||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@youknowriad @aduth I think this comment was lost, but I think we should use singular element
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mtias Copying my response from #284 (comment):
Singular element seems more appropriate here.
Is this in reference to the folder name? What about blocks ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aduth blocks has things like getBlocks
so seems a different abstraction. As far as I can tell element is more like a wrapper for a component abstraction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so rename folder to element
? I'm fine with that.
This pull request seeks to add some very rough folder structuring around blocks and complementary external modules. Much of this is based on conversations evolved from #104 and prototyped in
tinymce-per-block/src
.It includes non-implemented block APIs both in JavaScript and PHP with function signatures intentionally designed to mimic some similar already in WordPress (e.g.
register_rest_route
). Namespacing hadn't been considered before, but could be useful to prevent block name conflicts between plugin, such as if two plugins were to each attempt to register their ownmap
orcontactform
block types.Base settings for a block follow those explored at #104 (comment) , with assumption that blocks own state will serve as single source of truth, persisted in post meta and adhering to a structure defined by the block's schema defined in
register_block
.The idea with external modules is not only to help separate and isolate related logic, but also a means to enforce using standard APIs even in "first-party" blocks. The idea is that modules exist as an alternative to and in addition to window globals.