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

Group Block: Add support for margin control to the group block #33909

Closed
wants to merge 2 commits into from

Conversation

apeatling
Copy link
Contributor

@apeatling apeatling commented Aug 5, 2021

Description

Add support for controlling margins on the group block using the margin block support.

How has this been tested?

Tested locally.

Screenshots

2021-08-11 10 43 42

Types of changes

New feature

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@apeatling apeatling added the [Block] Group Affects the Group Block label Aug 5, 2021
@apeatling apeatling self-assigned this Aug 5, 2021
@apeatling apeatling added [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi [Type] Enhancement A suggestion for improvement. labels Aug 5, 2021
@ramonjd ramonjd mentioned this pull request Aug 6, 2021
7 tasks
@ramonjd
Copy link
Member

ramonjd commented Aug 6, 2021

I think this is a good change. 👍

At the very least, it opens up margin controls for many blocks that don't yet have margin block support (via nesting them in a group block).

It allows me to add margins, for example to images:

Screen Shot 2021-08-06 at 1 00 16 pm

I mentioned over in #33835 that nesting a cover block, for example, inside a group block with full spacing support allows great flexibility already.

And the behaviour is what I'd expect to see:

group-margin-cover.mp4
group-margin-image-full-width.mp4

Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

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

This generally tests well for me 👍

The only issue I see is the conflict between layouts support (#33359, #29335) and the new left/right margins in some themes and situations.

For example, take the TT1 Blocks theme. When the group block is a top-level block it will have a layout set that constrains the margin-left/right to auto !important. When it is a nested block it won't have the layout margins and so the margin support added in this PR will take effect.

I don't know how much of an issue this is although it is poor UX for users trying to adjust left/right margins and seeing nothing happening.

Being able to conditionally opt into block support is something that has come up before but previously it's been worked around. In this case, it would be good if we could adjust the group block's sides config for margin based on whether it was nested or not. If not nested, you could only adjust top and bottom margins.

Once the new ToolsPanel is merged (🤞 that is imminent), I think it might pay for the margin support control to be a non-default control to lower its prominence. This might mitigate some of the current conflicts between a group block's left/right margin when it's within a specific layout.

@ramonjd
Copy link
Member

ramonjd commented Aug 6, 2021

Being able to conditionally opt into block support is something that has come up before but previously it's been worked around.

That would be amazing, especially if we could, in this case, read the layout attribute.

@apeatling
Copy link
Contributor Author

Once the new ToolsPanel is merged (🤞 that is imminent), I think it might pay for the margin support control to be a non-default control to lower its prominence. This might mitigate some of the current conflicts between a group block's left/right margin when it's within a specific layout.

Agreed with this, I think it should be hidden by default, and always be secondary to any gap support in a parent block.

@apeatling
Copy link
Contributor Author

apeatling commented Aug 9, 2021

The only issue I see is the conflict between layouts support (#33359, #29335) and the new left/right margins in some themes and situations.

For example, take the TT1 Blocks theme. When the group block is a top-level block it will have a layout set that constrains the margin-left/right to auto !important. When it is a nested block it won't have the layout margins and so the margin support added in this PR will take effect.

I think this is a good reason for margin options to be hidden in the UI unless they are specifically accessed and used within the new ToolsPanel. I'd assume that the flex layout with gap support would be the first port of call and precede this. I was thinking that we'd work on this once this PR is in, since it can change across all blocks that opt in to this support. What do you think?

@aaronrobertshaw
Copy link
Contributor

I think this is a good reason for margin options to be hidden in the UI unless they are specifically accessed and used within the new ToolsPanel. I'd assume that the flex layout with gap support would be the first port of call and precede this. I was thinking that we'd work on this once this PR is in, since it can change across all blocks that opt in to this support. What do you think?

I personally don't have a strong opinion on merging this now. That's under the stated provision that the intention is not to display it by default once the ToolsPanel and dimension panel PR lands.

There has been a general desire however to manage the introduction of new controls to the sidebar. This might mean needing to hold off on merging this until the ToolsPanel actually lands.

@jasmussen Do you have any guidance on this one?

@jasmussen
Copy link
Contributor

I think this is a good reason for margin options to be hidden in the UI unless they are specifically accessed and used within the new ToolsPanel. I'd assume that the flex layout with gap support would be the first port of call and precede this. I was thinking that we'd work on this once this PR is in, since it can change across all blocks that opt in to this support. What do you think?

I'd agree with this. Lately I'm becoming a fan of the container handling many appearance properties for child blocks, and child blocks only overriding those when need be.

There has been a general desire however to manage the introduction of new controls to the sidebar. This might mean needing to hold off on merging this until the ToolsPanel actually lands.

Holding off margin until explicitly added by the tools panel feels implied in Andy's comment. Since the tools panel is so close to landing, though, it seems more a question of how soon after this panel would be swapped out with the fancy new one.

@apeatling
Copy link
Contributor Author

Holding off margin until explicitly added by the tools panel feels implied in Andy's comment. Since the tools panel is so close to landing, though, it seems more a question of how soon after this panel would be swapped out with the fancy new one.

Yes. Let's hold off on merging this until ToolsPanel is in and we can support that here.

@ramonjd
Copy link
Member

ramonjd commented Aug 11, 2021

Holding off margin until explicitly added by the tools panel feels implied in Andy's comment. Since the tools panel is so close to landing, though, it seems more a question of how soon after this panel would be swapped out with the fancy new one.

Yes. Let's hold off on merging this until ToolsPanel is in and we can support that here.

Making a note to self that I'll follow the same advice for the margin on the cover block.
#33835

@apeatling apeatling force-pushed the add/group-block-margin-support branch from 91a8a3b to d28ccf7 Compare August 11, 2021 17:24
@apeatling
Copy link
Contributor Author

I've now rebased this PR to include the new Dimensions panel using the ToolsPanel component. I've also added padding as the default expanded view, and left margin hidden and available via selection.

I think this should be good for another review. 👍

@stacimc
Copy link
Contributor

stacimc commented Aug 11, 2021

This is looking good to me! +1 for padding as a default control and margin hidden. I tested in the block and site editors, and tested setting values from theme.json.

I noticed in tt1-blocks that there's some theme styling that applies margin-top: 0; margin-bottom: 0 to Group blocks with a background color applied. This gets overridden successfully by per-block margin selections via the Dimensions panel, but will override margins set in global styles or in theme.json. I think this is slightly different from the issue with layouts already mentioned because it also applies to nested Group blocks, and because it causes a disparity between what's seen in the editor vs the frontend.

Eg:

  • In global styles (or theme.json, either work), add a 100px top and right margin, and a 20px bottom and left margin
  • Add a group block and select a background color
  • Insert a second group block inside the first and set a contrasting background color

In the editor you'll see that only the top and bottom margin were applied to the top-level block (this is the issue pointed out earlier), but all four margins were applied to the nested block:

Screen Shot 2021-08-11 at 1 14 48 PM

But on the frontend, none of the margins are applied to the top level block, and only the right and left margins are applied to the nested block:

Screen Shot 2021-08-11 at 1 15 06 PM

I'm not sure to what extent this should be considered a theme issue/an extension of the issue brought up earlier. I'm a bit worried about the disparity between what is seen in the editor vs the frontend, which is particularly confusing because sometimes it's top/bottom that "disappears" and sometimes it's left/right.

It is worth pointing out that margins added at the block level via the Dimensions panel do always work, so if a user notices this disparity they're still able to manually tweak it for each individual block. (Edit -- except for top-level Group blocks as already pointed out)

@aaronrobertshaw
Copy link
Contributor

I'm not sure to what extent this should be considered a theme issue/an extension of the issue brought up earlier.

@stacimc I don't think this is a theme specific issue as the style actually comes from the theme.scss file provided by the group block itself.

Apparently, these styles are part of the set that needs adjusting when the editor resizes. I'm not familiar with this so it could do with some further investigation.

If we need to ensure that the theme.json/global styles values are honoured in the editor perhaps the problem style could be updated to make use of the :where() CSS selector so it has a lower specificity than the generated theme.json/global styles.

@apeatling
Copy link
Contributor Author

Closing this one out because we are taking the block gap direction for now, but this may be revisited in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Group Affects the Group Block [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants