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

[WIP] Block Support: Add gap block support feature #32571

Conversation

andrewserong
Copy link
Contributor

@andrewserong andrewserong commented Jun 10, 2021

Update (2021-08-11): Now that the ToolsPanel (#32392) has been merged, the work from this PR has moved over to #33991.

@andrewserong andrewserong changed the title Add gap block support [WIP] Block Support: Add gap block support feature Jun 10, 2021
@vcanales
Copy link
Member

This is really cool, thanks for sharing!

WIP (and this PR is quite buggy right now)

It would be great if you could provide some testing instructions to reproduce these bugs; I'd like to fiddle around and help finding the solution.

@andrewserong
Copy link
Contributor Author

Thanks @vcanales, appreciate you taking an early look! This is very much an early WIP at the moment, but I've updated the testing instructions, hopefully that'll get you up an running. This PR depends on and was forked from #32392. I discovered while working on this that the BoxControl component in its current state doesn't quite meet our needs for the Gap support, because when the control is unlinked, when dealing with CSS Gap, we want to group together vertical and horizontal values and controls. Today, I took at look separately at adding support for that in the BoxControl component over in this PR: #32610

I'm wrapping up for the week, but hope to get back to this PR on Tuesday. In its current form, I just got the CSS property name construction working in packages/block-editor/src/hooks/style.js by adding an isReversedProperty flag. It might be a bit of an awkward way of constructing the property keys, but it seems to work so far. This'll need to be rolled out to global-styles-render.js too, I think. I also need to circle back to the PHP side and fix tests, etc.

I'm very happy for feedback of course, and please do leave any comments if there's a particular way you think this would be best implemented. 🙂

@vcanales
Copy link
Member

This is very much an early WIP at the moment, but I've updated the testing instructions, hopefully that'll get you up an running.

Thank you so much for updating the instructions! I understand the early status — I've recently worked on a few of those cool ideas that require a lot of tinkering to come to fruition. I'd like to approach this testing by using your proposed changes as a platform, so I'm not looking to nitpick or alter your current work, and rather "forking" it to look for stuff we might consider exploring. Thanks again!

@andrewserong
Copy link
Contributor Author

andrewserong commented Aug 3, 2021

I've just given this a manual rebase and adjusted a couple of things to get it compatible with the latest changes in #32392

It also now uses the splitOnAxis option in the BoxControl component to allow for grouped horizontal / vertical controls when the BoxControl is unlinked (which gives us better mapping to the columnGap and rowGap properties).

Still more to do (and I haven't looked at global styles yet), but this should now get this PR back in a mostly working state.

@andrewserong
Copy link
Contributor Author

I've updated this PR to add in global styles support, so it should be testable now to roughly cover the main areas that would be addressed by a gap support.

The work here might wind up being superseded by #33812, which has a more general solution for controlling gaps between blocks. I've added a comment over in that PR referencing this one: #33812 (comment)

CC: @youknowriad just pinging you here — this is my WIP exploring a gap support. I imagine we'll likely want to go with your PR instead (as the approach is more tied to layouts) and then iterate from there?

@andrewserong
Copy link
Contributor Author

Update: as discussed in #33812 (comment), I'll explore swapping out the inline gap styles for CSS variables based on that PR. Still to be determined: handling separate row / column values: e.g. do we need two CSS variables (column and row) to store values separately, or even three (to cover the short-hand)? I imagine two would do.

One of the big benefits of using CSS variables will be that we can defer to each block for working out how we want to use the value — whether we're ready to use CSS gap directly, or instead wish to use the value for calculating margins.

@andrewserong
Copy link
Contributor Author

I've updated this PR with a pass exploring using CSS variables instead of gap attributes directly. I think this will offer a lot more flexibility, but there are still some issues to address. I've added in the following CSS variables:

  • --wp--theme--block-gap — when using the short-hand value
  • --wp--theme--block-row-gap — when setting the row value
  • --wp--theme--block-column-gap — when setting the column value

I think these should be consolidated if possible, it's a little unwieldy having separate values and attempting to keep them in sync. However, it would still be good for blocks to be able to use separate row/column (or horizontal/vertical) values.

I've also updated the PR description with instructions on how to test using the Buttons block, by adding a couple of CSS rules.

Still to be done: I need to update some of the naming to keep this PR consistent (e.g. rename the gap support to blockGap).

@stacimc
Copy link
Contributor

stacimc commented Aug 10, 2021

I know this is still WIP, but this is looking great! I tested gap as well as the separated row/column gaps via:

  • setting through the Dimensions panel
  • updating via global styles
  • updating theme.json
  • double-checked changes persisted on the frontend

Pointing out just in case for others testing: there's a little bit of extra top/bottom margin being applied to individual Button blocks in the button styles, which could easily be removed when the support is added to Buttons 'officially' so that the row-gap is solely responsible for managing the space between rows.

Screen Shot 2021-08-10 at 2 54 42 PM

@aaronrobertshaw aaronrobertshaw deleted the branch WordPress:try/block-supports-dimensions-panel August 10, 2021 23:44
@andrewserong
Copy link
Contributor Author

Now that the Dimensions Panel PR has been merged (#32392), it looks like I'll need to open up a new PR to base this off trunk. I'll add a link when the new PR is up.

@andrewserong
Copy link
Contributor Author

Update: I have moved the changes from this PR over to #33991, which is based off trunk. I'll continue work over on that PR.

andrewserong added a commit that referenced this pull request Aug 13, 2021
andrewserong added a commit that referenced this pull request Aug 16, 2021
andrewserong added a commit that referenced this pull request Aug 27, 2021
andrewserong added a commit that referenced this pull request Sep 2, 2021
* Add gap block support feature based on #32571

* Server-render the gap block support, and skip serialization on save in the editor

* Add PHP tests for spacing gap support server-side rendering

* Rename support from customBlockGap to blockGap

* Align whitespace
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants