Inspector: Bootstrapping the block inspector API (Slot & Fill) #1050
Conversation
|
Nice! There doesn't seem to be a lot to see there yet, except when selecting the image block. I'm assuming this is intended! Can we have the name of the block in the breadcrumb trail? I.e. Post → Image instead of Post → Block?
Also, question: if we should decide that the inspector works best as a modal that you open from a button on the quick toolbar, as opposed to overlaying the sidebar, would this be a difficult change from this approach? |
|
Right! there's not much to see here aside the image block.
Yep, I expect this API to stay close to what we have in this PR. The wrapper And the header should show the block title now |
|
Stellar! ✨ |
youknowriad
force-pushed
the
add/block-inspector-api
branch
from
2177227 to
dad3a1e
Compare
|
@aduth do you have some thoughts on this? |
| render() { | ||
| const { label, value, onChange } = this.props; | ||
| const id = 'inspector-text-control-' + this.instanceId; | ||
| const updateValue = ( event ) => onChange( event.target.value ); |
There was a problem hiding this comment.
I'm sympathetic to the jsx-no-bind rule's reasoning despite finding it to be far too restricting at times. At least when we already have a component class, I think it might be reasonable to prefer creating callbacks as bound instance members.
blocks/library/image/index.js
Outdated
| isActive: ( { align } ) => 'right' === align, | ||
| onClick: toggleAlignment( 'right' ), | ||
| }, | ||
| { | ||
| icon: 'align-wide', | ||
| title: wp.i18n.__( 'Wide width' ), | ||
| icon: 'align-width', |
blocks/library/image/index.js
Outdated
|
|
||
| const inspector = focus && ( | ||
| <InspectorControls key="inspector"> | ||
| <TextControl label={ __( 'Alternate Text' ) } value={ alt } onChange={ updateAlt } /> |
There was a problem hiding this comment.
Wondering if there might be a pattern we can create here for making it easier to bind the attribute and change handlers, something like what LinkedStateMixin (deprecated) was meant to provide.
There was a problem hiding this comment.
I'm afraid if we come up with something like that, it would add a new thing to understand.
blocks/library/image/index.js
Outdated
|
|
||
| if ( ! url ) { | ||
| const uploadButtonProps = { isLarge: true }; | ||
| const setMediaURL = ( media ) => setAttributes( { url: media.url } ); | ||
| return ( | ||
| return [ | ||
| inspector, |
There was a problem hiding this comment.
Should we be showing inspector options (Alt Text) when the image is a placeholder?
There was a problem hiding this comment.
Aha! Interesting.
Probably not — we need to think about what shows on the frontend. And if you actually publish an image placeholder (forget to fill it out), I'm assuming you'll see nothing. But if we've offered you to fill out the alt text of a placeholder, feels like there's an implicit expectation that you should see something. So I do agree, probably we shouldn't show anything but the basic alignments for placeholders.
|
Merging this soon if no concerns. |
|
For me this completely broke the block inspector. I don't see anything for text blocks for example. |
|
@iseulde This is intended, the text block didn't declare any inspector control yet, we only have a control for image blocks right now. |
|
This also took me by surprise. :) If the goal is to still duplicate controls (alignment, formatting) we should render some placeholder so that it doesn't look entirely broken. If we don't want to do this in general, we should not show the inspector sidebar at all for blocks that don't register controls there. |
|
@mtias Could be a solution to hide the inspector entirely for blocks without inspector controls, but I didn't want to go too fast here because we may introduce generic inspector controls for all blocks (I'm thinking ID or class control for all blocks for example) |
Closes #672
editfunction of the blocks using theInspectorControlswrapper.TextControl.Once this merged, we'll have to create separate issues to implement the inspector for each block.