-
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
Add "Spacer" block to create empty areas. #6121
Conversation
Is there any reason why the Divider block can't just have a setting to make the divider invisible and add the ability to change the height of the block? |
blocks/library/spacer/editor.scss
Outdated
box-sizing: border-box; | ||
cursor: se-resize; | ||
left: 50% !important; | ||
margin-left: -7.5px; |
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.
Mixed whitespace.
blocks/library/spacer/editor.scss
Outdated
display: none; | ||
border-radius: 50%; | ||
border: 2px solid white; | ||
width: 15px !important; |
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 !important
? Can we just add more specificity if we're trying to defeat some other style?
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 coming from the re-resizable library — the image block does the same. We should wrap this library so this is handled for a block author, regardless of the !important.
blocks/library/spacer/index.js
Outdated
return [ | ||
<ResizableBox | ||
size={ { | ||
height: height, |
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.
Minor: Can take advantage of object property shorthand notation:
{
height,
}
blocks/library/spacer/index.js
Outdated
/>, | ||
isSelected && | ||
<InspectorControls key="inspector"> | ||
<input |
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.
Assume we want a proper labeled control here, though guessing this is in progress.
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.
Yes, we don't have an exposed control for a numbered input.
In using this it feels pretty natural and what I would expect from a spacer. The only issue I felt was if it's the top block or there is no space to grow above, it feels a little odd when you drag from the top but it grows from the bottom. I now also don't like this in images, but this totally could be a personal comment as I'm interacting and totally not a blocker. |
I dig it! |
@SuperGeniusZeb it sounds harder to discover and generally more convoluted than a separate single purpose block. We could add transforms from a separator to a spacer, though, that sounds very useful. |
core-blocks/spacer/index.js
Outdated
edit( { attributes, setAttributes, isSelected, toggleSelection } ) { | ||
const { height } = attributes; | ||
|
||
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.
I refactored all existing blocks to use <Fragment />
component instead of []
to avoid using key
attributes and to be consistent. Can we update this block, too?
What do you think about having edit
method in its own file? It was raised by the mobile team, as they experiment with replacing edit
method with RN alternatives. See #5816 for reference. It would help them immensely to iterate before we come up with a set of primitives and other restrictions (if ever).
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 "edit" we have to sort out block server registration. It could be a possibility.
core-blocks/spacer/index.js
Outdated
bottom: 'wp-block-spacer__resize-handler-bottom', | ||
} } | ||
enable={ { | ||
top: true, |
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.
Sidenote: so bad API which enforces you to provide all those false
values ...
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.
@gziolo yes :( At the very least we should abstract this with our own component. I also don't like the styles I had to supply.
We should look later in separate PR how to extract This component looks good 👍 |
In Progress
This PR adds a "spacer" block to add arbitrary space between blocks.
cc @melchoyce