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

Blockbase: Update Blockbase and children to use flex #4468

Merged
merged 7 commits into from
Aug 30, 2021

Conversation

scruffian
Copy link
Member

Changes proposed in this Pull Request:

Since https://github.com/WordPress/gutenberg/pull/33359/files merged we can use the flex property of the group block to achieve this layout. The blockGap setting doesn't seem to be working in my tests.

Related issue(s):

#4463

@scruffian scruffian requested a review from a team August 25, 2021 14:04
@scruffian scruffian self-assigned this Aug 25, 2021
@scruffian scruffian force-pushed the update/blockbase-use-flex branch from dfaf85a to 3e58ee4 Compare August 25, 2021 14:05
@MaggieCabrera
Copy link
Contributor

Mmmh, I think the gap was only applying initially to the normal layout, not the flex one (so it's actually a vertical gap between blocks inside a container, not horizontal)

@scruffian
Copy link
Member Author

blockGap should be inside styles not settings. This now works as expected.

@MaggieCabrera
Copy link
Contributor

Looks like Quadrat's header is affected by this (here):

Screenshot 2021-08-26 at 11 16 20

@scruffian
Copy link
Member Author

Good catch. Updated it!

Comment on lines 566 to 568
"spacing": {
"blockGap": "var(--wp--custom--margin--horizontal)"
},
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this might be too big:

Screenshot 2021-08-26 at 12 22 16

Screenshot 2021-08-26 at 12 23 27

This is also applying vertically to blocks inside layout inherit groups. The rule coming from here.

I think GB should have two different gap values for the two different layouts, since inherit is going to be separating vertically and flex is horizontal, which usually means smaller blocks of text that don't need such a big gap between them.

@scruffian scruffian force-pushed the update/blockbase-use-flex branch from 19e322b to 58521a1 Compare August 27, 2021 09:18
@scruffian
Copy link
Member Author

I've updated this to use the vertical margin everywhere. Ideally we'd be able to control the horizontal and vertical separately but for now I think this is an OK compromise.

Tracking here: WordPress/gutenberg#34347

@MaggieCabrera
Copy link
Contributor

Skatepark's post meta looks like this now:

Screenshot 2021-08-27 at 16 43 48

@pbking pbking force-pushed the update/blockbase-use-flex branch from 58521a1 to e8f5f13 Compare August 30, 2021 15:40
@pbking
Copy link
Contributor

pbking commented Aug 30, 2021

Fixed skatepark's meta:

Had to use an !important to beat the Gutenberg center style. That seems to opinionated to not have a way to configure.

@pbking
Copy link
Contributor

pbking commented Aug 30, 2021

NOW it looks like this:
image

@pbking
Copy link
Contributor

pbking commented Aug 30, 2021

post-meta is not a core block, it's just a class that we use for this bit of the theme :)

I just realized that and deleted my comment. :P

@MaggieCabrera
Copy link
Contributor

I just realized that and deleted my comment. :P

No one will know :D

@pbking
Copy link
Contributor

pbking commented Aug 30, 2021

I can't find anything else that doesn't look correct. I believe this is ready to come in.

Copy link
Contributor

@pbking pbking left a comment

Choose a reason for hiding this comment

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

LGTM
🚢

@pbking pbking merged commit a5e860f into trunk Aug 30, 2021
@scruffian
Copy link
Member Author

Thanks for finishing this one off @pbking

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants