-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Bootstrap some documentation for the block editor module #14566
Conversation
fc8fcf1
to
f4bf8b6
Compare
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.
It looks like a very nice improvement. I left some minor comments (mostly with typos) which should be addressed before the merge. There is also an open question left solely out of curiosity.
const [ blocks, updateBlocks ] = useState( [] ); | ||
|
||
return ( | ||
<BlockEditorProvider |
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.
Any plans to introduce another wrapping component to make it as simple as:
return (
<BlockEditor
value={ blocks }
onInput={ updateBlocks }
onChange={ updateBlocks }
>
<BlockList />
</BlockEditor
);
or something like that.
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.
I ultimately think all these wrappers I'm using here should be part of BlockEditorProvider
which is basically what you're proposing.
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.
Awesome, I like where it is heading 👍
Co-Authored-By: youknowriad <benguella@gmail.com>
<BlockEditorProvider | ||
value={ blocks } | ||
onInput={ updateBlocks } | ||
onChange={ updateBlocks } |
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.
For someone who doesn't care about "Undo" levels, I wonder if the simplest example can just include onInput
(or conversely, for one who cares only about important updates, only include onChange
).
Though as I write this, I guess it assumes that onInput
and onChange
aren't mutually exclusive (i.e. onInput
would always precede an onChange
). I think this was an original intention, but by the final implementation it didn't end up this way:
gutenberg/packages/block-editor/src/components/provider/index.js
Lines 95 to 99 in d9768ad
if ( isPersistent ) { | |
onChange( blocks ); | |
} else { | |
onInput( blocks ); | |
} |
The original thinking being that you could have onInput
and onChange
, or just individually one or the other of onInput
or onChange
.
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.
Good thoughts, and yeah I think you're right, it would have been better to have the two calls at the same time. I recall something related to the state making this harder for us but I think we can probably revisit specifically.
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.
IIRC, #14105 was partly toward this goal as well.
Related #14043
This PR enhances the README of the block editor module explaining how we can use it in a any context.