Skip to content
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

Merged
merged 3 commits into from
Mar 22, 2019

Conversation

youknowriad
Copy link
Contributor

Related #14043

This PR enhances the README of the block editor module explaining how we can use it in a any context.

@youknowriad youknowriad force-pushed the add/block-editor-docs branch from fc8fcf1 to f4bf8b6 Compare March 22, 2019 11:31
Copy link
Member

@gziolo gziolo left a 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
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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 👍

gziolo and others added 2 commits March 22, 2019 18:33
Co-Authored-By: youknowriad <benguella@gmail.com>
Co-Authored-By: youknowriad <benguella@gmail.com>
@youknowriad youknowriad merged commit ab136c2 into master Mar 22, 2019
@youknowriad youknowriad deleted the add/block-editor-docs branch March 22, 2019 17:58
<BlockEditorProvider
value={ blocks }
onInput={ updateBlocks }
onChange={ updateBlocks }
Copy link
Member

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:

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Developer Documentation Documentation for developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants