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.

Sign up for GitHub

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 padding support to Group block #24966

Merged
merged 2 commits into from
Sep 3, 2020
Merged

Add padding support to Group block #24966

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
69f1971
Add padding support to Group block
youknowriad Sep 1, 2020
4d2c35e
Fix UnitControl rendering with Tooltip in BoxControl
Sep 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions packages/block-library/src/editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,14 @@
margin-bottom: $default-block-margin;
}

.block-editor-block-list__layout > .wp-block:first-child {
margin-top: 0;
}

.block-editor-block-list__layout > .wp-block:last-child {
margin-bottom: 0;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasmussen The idea here is that since we're defining the base margin above, we need to reset these margins for the first and last block of each container block.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this, the padding is not applied properly in the group block (you'll notice an extra space on tthe editor)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As can be seen in #24966 (comment), this isn't working for me. I have the following in my editor style:

figure,
p {
	margin-top: 2.5rem;
	margin-bottom: 2.5rem;
}

I'm still sort of worried about this being an awfully specific style rule that could have consequences we can't imagine. I don't have a strong opinion, insofar as I appreciate you trying to handle this so I wouldn't have to manually go to the last paragraph and specify a zero margin. I suppose we can try it, and remove it if it has side effects.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid this had unintended consequences.

appender

The above happens because every nesting container has a last child that's the appender:

Screenshot 2020-09-22 at 09 36 51

Unless, apparently, an adjacent block has been selected:

Screenshot 2020-09-22 at 09 37 00

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 guess that's because the appender div is the "last child" so the margin reset is not applied. I wonder why this empty div is rendered and whether we can remove it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this PR, I remove the rules: #25527 — I honestly think it's for the best.

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'll take a look but I think there's still value in the rules. So I'd like to see if we can find a better solution.


// This tag marks the end of the styles that apply to editing canvas contents and need to be manipulated when we resize the editor.
#end-resizable-editor-section {
display: none;
Expand Down
3 changes: 2 additions & 1 deletion packages/block-library/src/group/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
"__experimentalColor": {
"gradients": true,
"linkColor": true
}
},
"__experimentalPadding": true
}
}
6 changes: 6 additions & 0 deletions packages/block-library/src/group/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import {
InnerBlocks,
__experimentalBlock as Block,
} from '@wordpress/block-editor';
import { __experimentalBoxControl as BoxControl } from '@wordpress/components';
const { __Visualizer: BoxControlVisualizer } = BoxControl;

function GroupEdit( { attributes, className, clientId } ) {
const hasInnerBlocks = useSelect(
Expand All @@ -20,6 +22,10 @@ function GroupEdit( { attributes, className, clientId } ) {

return (
<BlockWrapper className={ className }>
<BoxControlVisualizer
values={ attributes.style?.spacing?.padding }
showValues={ attributes.style?.visualizers?.padding }
/>
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 think this can probably be absorbed by the hook and added automatically somehow. cc @ItsJonQ

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ItsJonQ I'm not familiar with the attribute shape yet, can you clarify a bit what visualizers contain here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice if it would be absorbed by the hook, otherwise we need to be more careful with any updates to BoxControlVisualizer. Also from its name, it feels like it shouldn't be a public API.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@youknowriad Sure! I can expand on this a bit. The visualizers need to show in 2 ways:

  1. On value change (they flash) (via the values prop)
  2. On hover (they persist) (via the showValues prop, which are boolean values)

Right now, state for showValues object is being communicated from the controls (in the sidebar) to the canvas via updating the style attribute.

It works, but I'd prefer a more streamlined way of coordinating this state - Ideally without using the data store and relying on a context + Provider wrapper.

Hope this helps!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm a bit concerned about the use of an attribute for that state since it's not a persistent state.
I wonder if we could just rely on local state if we move the rendering of the BoxControlVisualizer to the hook. Not sure thought.

<InnerBlocks
renderAppender={
hasInnerBlocks
Expand Down
3 changes: 0 additions & 3 deletions packages/block-library/src/group/editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,6 @@
*/

.wp-block-group {
// Zero out the baseline margin that is set for every block in the editor.
margin-top: 0;
margin-bottom: 0;
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 think this is not needed with my change here https://github.com/WordPress/gutenberg/pull/24966/files#r481034215


// Ensure not rendering outside the element
// as -1px causes overflow-x scrollbars
Expand Down
10 changes: 9 additions & 1 deletion packages/components/src/box-control/unit-control.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,17 @@ export default function BoxUnitControl( {
function Tooltip( { children, text } ) {
if ( ! text ) return children;

/**
* Wrapping the children in a `<div />` as Tooltip as it attempts
* to render the <UnitControl />. Using a plain `<div />` appears to
* resolve this issue.
*
* Originally discovered and referenced here:
* https://github.com/WordPress/gutenberg/pull/24966#issuecomment-685875026
*/
return (
<BaseTooltip text={ text } position="top">
{ children }
<div>{ children }</div>
</BaseTooltip>
);
}