-
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
Implement image block placeholder #566
Conversation
I think this could still be useful for laying out a page, and adding the images afterwards. |
blocks/library/image/style.scss
Outdated
.blocks-image__placeholder-label { | ||
display: flex; | ||
align-items: center; | ||
justify-content: center; |
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.
Looks like spacing was added by mistake
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.
Looks like spacing was added by mistake
Thanks! Fixed in the rebased e4b84ea.
Working great! |
Yes, the alignment controls are totally fine on the placeholder. Imagine cases where an editor for a publication lays out the structure of a post, but leaves it up to other people to fill the media. Also could be useful for page templates. |
@@ -72,6 +75,23 @@ registerBlock( 'core/image', { | |||
edit( { attributes, setAttributes, focus, setFocus } ) { | |||
const { url, alt, caption } = attributes; | |||
|
|||
if ( ! url ) { | |||
return ( |
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.
Would it make sense to move this into an empty
function?
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.
Would it make sense to move this into an
empty
function?
What does "empty" mean?
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 means when it acts as a placeholder, when no content is yet set. This is going to be a prevalent case for templates down the road.
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 means when it acts as a placeholder, when no content is yet set. This is going to be a prevalent case for templates down the road.
Right, but in the sense of treating blocks as rendering always from its state, an imperative "renderNew" doesn't make sense. A block could implement conditions under which it could be considered empty (here, ! url
), but it's unclear how we'd want to generalize this. Maybe related to #609, an attribute can be flagged as required
and, when missing, use a separate render function like your empty
example.
I don't know that introducing this break from edit
/ save
duality really provide a whole lot of value though for what could be equally represented by:
edit: ( { attributes } ) =>
attributes.url
? <ImageEdit />
: <EmptyImageEdit />
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.
That last example looks alright, but I'm mostly concerned with people doing all sort of different things for something that should have a consistent UX. Empty could be just the comment (leaving blocks to implement more fine grained UI handling when specific attributes are missing), which could be a nice way to define a template:
<!-- wp:core/text --><!-- /wp:core/text -->
<!-- wp:core/image --><!-- /wp:core/image -->
<!-- wp:core/button --><!-- /wp:core/button -->
In any case I think forcing block authors to have a separate function for returning the empty render, regardless of whether it is called from edit
manually, could be helpful.
412999d
to
e4b84ea
Compare
Closes #454
This pull request seeks to implement the placeholder image block. It is currently not functional; there are no behaviors to drop or insert an image from the media library.
Open questions:
controls
depending on its current attributes. We could do this by allowing the block implementer to optionally specifycontrols
as a function. Or, depending on how far we want to take the Slot & Fill idea (Try Slot & Fill pattern for rendering Editable formatting controls #507), assume that a block must render its own controls via<Fill>
, meaning they'd have full control over the rendering from theedit
function.Testing instructions:
Verify that after inserting a new image block, a new block is added with a placeholder appearance.