Columns - fix margins, keep 2-col breakpoint #12816

drdogbot7 · 2018-12-12T16:15:42Z

This is a revision of #12408 which includes better comments and also applies the same fixes in the editor.

Background

There should be 3 breakpoints:

Mobile: less than 600px
all columns full width and stacked. No margins.

Small: 600px - 782px
columns wrap in rows of 2. Margins on the inside.

Medium: bigger than 782px
all columns fit in one row. Margins on the inside.

PROBLEMS!

PROBLEM 1
Columns don't wrap properly at Small breakpoint

SOLUTION

move not selectors to Medium breakpoint
adjust flex-basis to account for margins using calc()

PROBLEM 2
At the Medium breakpoint, when there is an odd number of columns, and extra margin is added to the last item.

SOLUTION

reset odd and even selectors to initial at the Medium breakpoint

How has this been tested?

tested in local wordpress docker env

ORIG:
https://codepen.io/drdogbot7/pen/NEOQPQ

FIX:
https://codepen.io/drdogbot7/pen/GwYVNz

Types of changes

Only css affecting core/columns.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.

closes #13035

Columns were broken b/w the small and medium breakpoints, and when there are an odd number of columns. This fixes those issues. orig: https://codepen.io/drdogbot7/pen/NEOQPQ Fix: https://codepen.io/drdogbot7/pen/GwYVNz

jasmussen

This is such a nice pull request. Meaning to address a specific problem, well documented, lots of thoughts put into it. Thanks so much. Once we get this in you should be sure to add yourself to the contributors.md doc if you'd like and if you're not already there.

How does this one relate to #12199? Should it be merged along with the other, or is one of them sufficient?

I took this for a spin and especially on the frontend this seems to work well:

However there seems to be a small discrepancy in the editor view. Here's 5.0.1:

Here's this branch:

It's the same in 2019:

I'm not sure quite what it is, the CSS seems to be on point. Can you reproduce? I saw this with 2 columns.

I have some comments about the code comments as well, but I will leave those separately. But overall, this is a great PR and with a few small tweaks I think we should get it in!

jasmussen · 2018-12-17T07:52:22Z

packages/block-library/src/columns/editor.scss

@@ -89,13 +89,17 @@

 			// Beyond mobile, allow 2 columns.
 			@include break-small() {
-				flex-basis: 50%;
+				// Size should be half the available width, minus the margins added below


@tofumatt has taught me to be consistent (going forward, I was a bad actor for a long time) with our code comment system. Which is capital first letter, end with a period.

In this case we're missing a period at the end.

jasmussen · 2018-12-17T07:52:45Z

packages/block-library/src/columns/editor.scss

@@ -89,13 +89,17 @@

 			// Beyond mobile, allow 2 columns.
 			@include break-small() {
-				flex-basis: 50%;
+				// Size should be half the available width, minus the margins added below
+				// In the editor, we also need to account for additional block padding


Same here. I know this is really nitpicky, but we're basically a margin fix and some code comments polish from shipping this.

jasmussen · 2018-12-17T07:52:56Z

packages/block-library/src/columns/editor.scss

+				&:nth-child(odd) {
+					margin-right: initial;
+				}
+				// Add left margin to all but first column


Needs a period.

jasmussen · 2018-12-17T07:53:04Z

packages/block-library/src/columns/editor.scss

 				&:not(:first-child) {
 					margin-left: $grid-size-large * 2;
 				}
-
+				// add right margin to all but last column


Capital first letter and period.

jasmussen · 2018-12-17T07:53:23Z

packages/block-library/src/columns/style.scss

@@ -18,7 +18,9 @@

 	// Beyond mobile, allow 2 columns.
 	@include break-small() {
-		flex-basis: 50%;
+
+		// Size should be half the available width, minus the margins added below


Needs a period. Hey, actually I think I'll stop these reviews and just push a fix. Hold on.

jasmussen · 2018-12-17T08:05:34Z

packages/block-library/src/columns/editor.scss

-				flex-basis: 50%;
+				// Size should be half the available width, minus the margins added below.
+				// In the editor, we also need to account for additional block padding.
+				flex-basis: calc(50% - #{$grid-size-large * 2} - #{$block-padding * 2});


I took a stab at fixing the margin issue I noted. When I reverted this back to flex-basis: 50%; (editor only), I got things working:

Keep in mind that the editor uses a lot of negative margins left and right so as to make available a selectable/hoverable space left and right of blocks. This might be why the additional grid stuff isn't necessary here. Am I missing something?

This is with flex-basis: 50%;:

Again, I have no doubt that this is necessary on the front-end, but I think it's what's causing issues in the editor.

Hm. When I change this back to flex-basis: 50%;, this is what I get at the Small (600px) breakpoint.

Did you have to make any other changes?

Ah good call. But if you change "break-medium" back to "break-small" here: https://github.com/WordPress/gutenberg/pull/12816/files#diff-49ac41bc72e24e343a451fe254801c0fR38 — it works again:

At the small (2-column) breakpoint… I think as long as there's a margin b/w the columns, then flex-basis needs to account for that doesn't it? 50% plus any margin will always wrap after the first column, right?

Possibly. But keep in mind that there is negative margins applied to columns in the editor only, to ensure that this looks right. Keep in mind the markup in the editor is different from the markup on the frontend — there are a lot more divs in the editor to enable the various tools for each block. So until we have access to something like display: contents; there's likely always going to be a discrepancy between the frontend and backend.

Did you try flex-basis: 50%? For me it seemed to fix the issue of the extra right margin, but there could be situations I haven't accounted for there.

For me flex-basis:50%; fixes the extra margin at the medium breakpoint, but only at the expense of breaking 2-column layout at the small breakpoint.

I think the problem was actually that at the medium breakpoint, I had reset the ODD/EVEN margins to initial. That's correct on the front-end, but in the editor, they should be set to $block-padding.

It gets a little confusing with all the nesting, but I think that's right.

For me flex-basis:50%; fixes the extra margin at the medium breakpoint, but only at the expense of breaking 2-column layout at the small breakpoint.

This only works if you also set the wrap property to nowrap as noted in #12816 (comment).

But I'm going to take a look at your latest push, see if that fixes it!

jasmussen · 2018-12-17T08:05:58Z

Pushed some polish to the comments, and left an idea for a solution in https://github.com/WordPress/gutenberg/pull/12816/files#r242051845.

drdogbot7 · 2018-12-17T18:00:30Z

I pushed a new commit which works for me.

In the editor I get 2 columns at the small breakpoint and no extra margins at the medium breakpoint.

I adjusted the margins so that we get the same spacing b/w columns (64px) in the editor and front-end.

I don't love using Calc b/c it feels a little dirty to me, but I don't see a better way to get 2 even columns.

Something like this might be ideal in the future, but it only works in FF currently.

@include break-small() {
	flex: 1;
	&::nth-child(2n) {
		page-break-after: always;
		break-after: always;
	}
}

One issue I still have is that the vertical spacing b/w rows is a little tight.

The front-end CSS adds a margin-bottom:1em; to each column, so maybe we ought to add that in the editor as well?

jasmussen · 2018-12-17T18:44:59Z

The latest change seems to improve things a fair bit at the 2 column breakpoint.

But the margins seem too big now, compared to master. This branch:

Master:

There's also an issue with many columns:

Here's master:

It's a big headache, I know. And it's getting late for me here in Denmark. I promise I'll take a good long stab at this tomorrow, look at your latest changes, and see if I can fix it up.

jasmussen · 2018-12-17T18:46:34Z

packages/block-library/src/columns/editor.scss

@@ -35,7 +35,7 @@
 		// Responsiveness: Allow wrapping on mobile.
 		flex-wrap: wrap;

-		@include break-small() {
+		@include break-medium() {


We discussed this a little bit already, but what is the context for changing this to medium?

To my understanding the ideal situation is this:

Mobile (small) breakpoint has 1 column

Medium breakpoint has 2 columns

Above Medium has user set amount of columns.

To me this says that Mobile should wrap, but anything above mobile should not wrap.

what is the context for changing this to medium?

The reason to change this to medium is so that the small-to-medium breakpoint (600px - 782px) can wrap after the EVEN columns.

Mobile: 1 (wrap) 2 (wrap) 3 (wrap) 4 (wrap) 5 Small to Medium 1, 2 (wrap) 3, 4 (wrap) 5  Medium and up 1, 2, 3, 4, 5

If we set 'nowrap' at the Small breakpoint (600px), then the small-to-medium breakpoint doesn't work. flex-wrap:nowrap will prevent the columns from wrapping.

Mobile: 1 (wrap) 2 (wrap) 3 (wrap) 4 (wrap) 5  Small to Medium 1, 2, 3, 4, 5 Medium and up 1, 2, 3, 4, 5

I'm not opposed to removing the small-to-medium breakpoint entirely; I just didn't think that was the original intent.

That's a solid point. Thank you for clarifying, and for the patience.

- Reduce the margin between columns. This fixes an overlap issue at 6 columns wide. - Improve the wrapping and mobile, by adding a bottom margin to the column container.

jasmussen · 2018-12-18T08:40:26Z

Okay, I think the branch is now in a good state. I pushed a few small tweaks.

I discovered at the wide alignments there was no overlap as previously noted here:

But it was present when not wide:

This suggested that the problem was lack of room, more than any other thing. So I reduced the margins, which I have come around to, were too large regardless. This is a big PR, so I think it's good to make this change along with it, especially as it solves a problem.

There was an overlap issue at the 2 column breakpoint:

This was because flex causes a new margin context, which means the block margins collapse. In other words, because we wrap the column blocks (singular), those now need a bottom margin. I added that:

This was also necessary for mobile:

This is now the frontend:

And this is the backend:

I think this is ready for a wider code review and good testing. So I'm expanding the range.

I have addressed my own feedback points. Now awaiting others' reviews.

drdogbot7

Working for me. Looks consistent between FE and Editor. 👍

m-e-h · 2018-12-18T15:22:59Z

packages/block-library/src/columns/editor.scss

+			// Beyond the medium breakpoint, show the user-set amount of columns.
+			@include break-medium() {
+				// Because columns do not wrap, reset the column margin.
+				margin-bottom: initial;


initial (also used in style.scss) should probably be replaced with the actual value of 0 for better browser support.
This was news to me not too long ago but IE doesn't support initial.

Looks like postcss or something else is actually converting it to 0 in the final CSS anyway. Still probably a good idea to avoid the abstraction here.

Still probably a good idea to avoid the abstraction here.

Makes sense to me. I guess using initial doesn't really get us anything.

m-e-h · 2018-12-18T17:31:42Z

packages/block-library/src/columns/style.scss

@@ -18,7 +18,9 @@



Should we change the shorthand flex: 1 to just the property we're using from it, flex-grow: 1, here?

A little redundant. This is essentially what we currently have:

.wp-block-column { flex: 1; (flex-grow: 1) (flex-basis: 0%) flex-basis: 100%; }

Good call there I'd say. I still always have to lookup what Flex:1;actually means.

Actually, I don't think we need to set flex:1; or flex-grow:1; at all. flex-basis:100%; (or width:100%;) is all we need.

m-e-h · 2018-12-18T18:16:44Z

packages/block-library/src/columns/style.scss

+	}
+
+	// All columns fit in one row at this breakpoint.
+	@include break-medium() {


This even, child, nth, first, not reset stuff makes me dizzy 😵

Any interest in simplifying this?

Something along these lines maybe:

@include break-medium() { // Fill in the left and right margin holes left by the previous small breakpoint. // First column already has a margin-right and no margin-left. &:not(:first-child) { margin-left: $grid-size-large; margin-right: $grid-size-large; } // Remove right margin on last column. &:last-child { margin-right: 0; } }

I see the value in simplifying things, but I also see the value in not changing the logic once a block is already in the wild. I'd defer to @jasmussen and others on this.

m-e-h · 2018-12-18T23:31:33Z

There's no real benefit to this, honestly.

I may have gotten a little carried away trying make it simple. And it still looks complicated. 😀

jasmussen · 2018-12-19T07:46:30Z

I love the great feedback on this thread. In this particular case, I applaud the simplification.

It's worth noting that the smaller the change we make, the more likely it is to get in soon. Already now this is sort of a big change, though we can mitigate that with thorough testing:

Mobile, medium, large breakpoints
Frontend and backend both
Physical iphones/androids (or the emulators/simulators)
Modern browsers
IE11 and Edge

A note on IE11 — I recall columns working there, but the experience doesn't have to be top tier, it just has to not be broken.

drdogbot7 · 2018-12-19T15:41:01Z

In this particular case, I applaud the simplification.

If we want to simplify and make it easy to understand, and we're willing to make significant changes then:

Remove the :not rules. I like how clever they are, but they confused me at first, and I know I'm not the only one.
Remove the Small-to-medium breakpoint. That seems to be the source of a lot of the complexity. Removing it will make things easier on themers who want to customize, and it currently doesn't work, so removing it shouldn't inconvenience too many people.

Now here's our entire style.scss

.wp-block-columns {
	display: flex;

	// Responsiveness: Allow wrapping on mobile.
	flex-wrap: wrap;

	@include break-medium() {
		flex-wrap: nowrap;
	}
}

.wp-block-column {
	width: 100%; // works just like flex-basis here, and it's easier to override
	margin-bottom: 1em;

	// Prevent the columns from growing wider than their distributed sizes.
	min-width: 0;

	// Prevent long unbroken words from overflowing.
	word-break: break-word; // For back-compat.
	overflow-wrap: break-word; // New standard.

	// User defined # of columns beyond medium breakpoint.
	@include break-medium() {
		margin-left: $grid-size-large;
		margin-right: $grid-size-large;

		// Remove outside margin from first and last
		&:first-child {
			margin-left: 0;
		}

		&:last-child {
			margin-right: 0;
		}
	}
}

It's worth noting that the smaller the change we make, the more likely it is to get in soon.

This was my thinking originally, but if we want to really simplify, then let's do it.

EDIT:
If we do want to keep the small-to-medium breakpoint, then maybe do it like this:

// Between small and medium breakpoints, allow only two columns.
@media (min-width: #{ ($break-small) }) and (max-width: #{ ($break-medium) }) {

	&:nth-child(even) {
		margin-left: $grid-size-large;
	}

	&:nth-child(odd) {
		margin-right: $grid-size-large;
	}

	// Size should be half the available width, minus the margins.
	max-width: calc(50% - #{ $grid-size-large });
}

Now it's more clear what those styles are doing, and we don't have to override our ODD/EVEN rules later.

m-e-h · 2018-12-19T17:38:07Z

That's similar to how the text-columns are doing it. https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/text-columns/style.scss

Skinning this cat could go on and on. Since the columns are actually "broken" currently, we should probably get a fix in sooner than later. We can iterate and improve the code after that if needed.

Is there still a text-column block, btw? Not sure I've ever seen it...

jasmussen · 2018-12-19T17:43:49Z

So, that brings us to the big question. When can we ship this?

We have 5.0.x releases, which are supposed to be really small or really critical. And we have major releases which can be bigger, like 5.1. The latter obviously is a while out.

@youknowriad if you have time — what are the chances for this pull request to make it to a minor release? And if it's too big as is, how small would it have to be to land in a minor release?

youknowriad · 2018-12-20T09:03:56Z

I think this can go in 5.1 which is targeted for February. At this point it's not certain whether there will be a 5.0.3 before that.

…flex-basis to width

drdogbot7 · 2018-12-20T17:16:24Z

I pushed a small update.

Changed margin-*: initial; to margin-*: 0;, as per @m-e-h's comment.
Removed unnecessary flex: 1; and flex-grow: 0;
Changed flex-basis to width. It does the same thing, is slightly more compatible (IE), and is easier to override. (content < width < flex-basis).

I did basic testing of editor and FE in Chrome, Safari, Firefox (all mac), as well as IE11.

mapk

Looking really good!

I tested this with a few different themes. Take a look at these gifs. The first two themes still showed an indention issue, but the last theme, Twenty Nineteen, appeared fine.

https://cloudup.com/cl64LZYWXyZ

I tested on MacBook Pro, Chrome 71.0.

I'm not entirely sure if this is a theme issue, or something else. If I can help with providing more info, please let me know.

m-e-h · 2019-01-09T16:31:56Z

I mostly agree @WraithKenny . I at one point had a version which eliminated the even, odd stuff.
The problem with the negative margins on the wrapper though, is that a common pattern I'm seeing themes use for blocks is something similar to this:

.entry-content > * {
    max-width: 900px;
    margin-left: auto;
    margin-right: auto;
}

In this case, a negative left margin would take this block out of the center and move it to the left of the screen.

Like I said though, I do agree. There have got to be simpler methods.

drdogbot7 · 2019-01-16T23:21:01Z

This is all needlessly complex and with too many rules that themes have to overwrite to fix.

I do agree with @WraithKenny and @m-e-h that simpler would be better for the sake of theming, but the point of this PR is just to fix the bugs that were present in the initial release, not to make any other changes.

drdogbot7 · 2019-01-16T23:51:07Z

Looking really good!

I tested this with a few different themes. Take a look at these gifs. The first two themes still showed an indention issue, but the last theme, Twenty Nineteen, appeared fine.

https://cloudup.com/cl64LZYWXyZ

I tested on MacBook Pro, Chrome 71.0.

I'm not entirely sure if this is a theme issue, or something else. If I can help with providing more info, please let me know.

I just tested in Atomic Theme and Monostack and I couldn't replicate these issues that @mapk had. Possibly themes have been updated?

Atomic Theme

Monostack:

drdogbot7 · 2019-01-30T17:34:01Z

Closing this to avoid confusion. #12199 has similar fixes.

drdogbot7 added 4 commits December 3, 2018 12:43

Update style.scss

f7c8f1f

Columns were broken b/w the small and medium breakpoints, and when there are an odd number of columns. This fixes those issues. orig: https://codepen.io/drdogbot7/pen/NEOQPQ Fix: https://codepen.io/drdogbot7/pen/GwYVNz

clarify comments

2cb1ffb

apply fix in editor

4f54644

clarify comments

cd9873d

drdogbot7 mentioned this pull request Dec 12, 2018

Remove unintentional right-margin on last odd-item. #12199

Merged

4 tasks

gziolo added the [Block] Columns Affects the Columns Block label Dec 14, 2018

gziolo requested a review from jasmussen December 14, 2018 10:24

gziolo added the [Type] Enhancement A suggestion for improvement. label Dec 14, 2018

jasmussen previously requested changes Dec 17, 2018

View reviewed changes

jasmussen reviewed Dec 17, 2018

View reviewed changes

Polish up comments.

9ccb87d

jasmussen reviewed Dec 17, 2018

View reviewed changes

more consistent block padding

78c58d3

jasmussen reviewed Dec 17, 2018

View reviewed changes

Push a few fixes:

4ab8745

- Reduce the margin between columns. This fixes an overlap issue at 6 columns wide. - Improve the wrapping and mobile, by adding a bottom margin to the column container.

jasmussen requested review from a team and jasmussen December 18, 2018 08:41

drdogbot7 commented Dec 18, 2018

View reviewed changes

m-e-h requested changes Dec 18, 2018

View reviewed changes

m-e-h reviewed Dec 18, 2018

View reviewed changes

youknowriad added this to the 4.9 milestone Dec 20, 2018

set margins to 0 not initial, rm unnecessary flex properties, change …

28cd81d

…flex-basis to width

drdogbot7 mentioned this pull request Dec 20, 2018

Style issue of columns #13035

Closed

mapk reviewed Dec 31, 2018

View reviewed changes

youknowriad mentioned this pull request Jan 9, 2019

Gutenberg Columns Possible Fix #13252

Closed

kjellr mentioned this pull request Jan 10, 2019

Two column block: sub-blocks not aligned in mobile view WordPress/twentynineteen#742

Closed

youknowriad modified the milestones: 4.9 (Gutenberg), 5.0 (Gutenberg) Jan 21, 2019

drdogbot7 mentioned this pull request Jan 30, 2019

remove broken 2-column breakpoint from columns block #13605

Closed

5 tasks

drdogbot7 closed this Jan 30, 2019

Columns - fix margins, keep 2-col breakpoint #12816

Columns - fix margins, keep 2-col breakpoint #12816

Conversation

drdogbot7 commented Dec 12, 2018 • edited by youknowriad Loading

Background

PROBLEMS!

How has this been tested?

Types of changes

Checklist:

jasmussen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmussen commented Dec 17, 2018

drdogbot7 commented Dec 17, 2018

jasmussen commented Dec 17, 2018

jasmussen Dec 17, 2018 • edited Loading

Choose a reason for hiding this comment

drdogbot7 Dec 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmussen commented Dec 18, 2018

drdogbot7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-e-h Dec 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m-e-h commented Dec 18, 2018

jasmussen commented Dec 19, 2018

drdogbot7 commented Dec 19, 2018 • edited Loading

m-e-h commented Dec 19, 2018 • edited Loading

jasmussen commented Dec 19, 2018

youknowriad commented Dec 20, 2018

drdogbot7 commented Dec 20, 2018

mapk left a comment • edited Loading

Choose a reason for hiding this comment

m-e-h commented Jan 9, 2019

drdogbot7 commented Jan 16, 2019

drdogbot7 commented Jan 16, 2019

drdogbot7 commented Jan 30, 2019

drdogbot7 commented Dec 12, 2018 •

edited by youknowriad

Loading

jasmussen Dec 17, 2018 •

edited

Loading

drdogbot7 Dec 17, 2018 •

edited

Loading

m-e-h Dec 18, 2018 •

edited

Loading

drdogbot7 commented Dec 19, 2018 •

edited

Loading

m-e-h commented Dec 19, 2018 •

edited

Loading

mapk left a comment •

edited

Loading