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

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

Closed
wants to merge 8 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f7c8f1f
Update style.scss
drdogbot7 Nov 29, 2018
2cb1ffb
clarify comments
drdogbot7 Dec 11, 2018
4f54644
apply fix in editor
drdogbot7 Dec 12, 2018
cd9873d
clarify comments
drdogbot7 Dec 12, 2018
9ccb87d
Polish up comments.
Dec 17, 2018
78c58d3
more consistent block padding
drdogbot7 Dec 17, 2018
4ab8745
Push a few fixes:
Dec 18, 2018
28cd81d
set margins to 0 not initial, rm unnecessary flex properties, change …
drdogbot7 Dec 20, 2018
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
22 changes: 19 additions & 3 deletions packages/block-library/src/columns/editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
// Responsiveness: Allow wrapping on mobile.
flex-wrap: wrap;

@include break-small() {
@include break-medium() {
Copy link
Contributor

@jasmussen jasmussen Dec 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Author

@drdogbot7 drdogbot7 Dec 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

<!-- no wrap -->

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

<!-- no wrap -->

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

flex-wrap: nowrap;
}

Expand Down Expand Up @@ -89,27 +89,43 @@

// 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.
flex-basis: calc(50% - #{$grid-size-large * 2} - #{$block-padding * 2});
Copy link
Contributor

Choose a reason for hiding this comment

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

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:

screenshot 2018-12-17 at 09 03 05

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%;:

screenshot 2018-12-17 at 09 03 05

screenshot 2018-12-17 at 09 04 47

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.

Copy link
Author

Choose a reason for hiding this comment

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

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?

image

Copy link
Contributor

Choose a reason for hiding this comment

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

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:

screenshot 2018-12-17 at 16 51 43

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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!

flex-grow: 0;
}

// Add space between columns. Themes can customize this if they wish to work differently.
// This has to match the same padding applied in style.scss.
// Only apply this beyond the mobile breakpoint, as there's only a single column on mobile.

// 2 columns at this breakpoint, and items beyond the second will wrap.
@include break-small() {
&:nth-child(odd) {
margin-right: $grid-size-large * 2;
}

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

@include break-small() {
@include break-medium() {
// Reset odd, even margins from small breakpoint.
&:nth-child(even) {
margin-left: initial;
}

&:nth-child(odd) {
margin-right: initial;
}

// Add left margin to all but first column
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs a period.

Copy link
Author

Choose a reason for hiding this comment

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

thanks.

&:not(:first-child) {
margin-left: $grid-size-large * 2;
}

// Add right margin to all but last column.
&:not(:last-child) {
margin-right: $grid-size-large * 2;
}
Expand Down
23 changes: 21 additions & 2 deletions packages/block-library/src/columns/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@

Copy link
Member

Choose a reason for hiding this comment

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

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%;
}

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

// Beyond mobile, allow 2 columns.
@include break-small() {
flex-basis: 50%;

// Size should be half the available width, minus the margins added below.
flex-basis: calc(50% - #{$grid-size-large * 2});
flex-grow: 0;
}

Expand All @@ -31,18 +33,35 @@

// Add space between columns. Themes can customize this if they wish to work differently.
// Only apply this beyond the mobile breakpoint, as there's only a single column on mobile.

// 2 columns at this breakpoint, and items beyond the second will wrap.
@include break-small() {
&:nth-child(even) {
margin-left: $grid-size-large * 2;
}

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

// All columns fit in one row at this breakpoint.
@include break-medium() {
Copy link
Member

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

Choose a reason for hiding this comment

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

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;
		}
	}

Copy link
Author

Choose a reason for hiding this comment

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

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.

// Reset odd, even margins from small breakpoint.
&:nth-child(even) {
margin-left: $grid-size-large * 2;
margin-left: initial;
}

&:nth-child(odd) {
margin-right: initial;
}

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

// Add right margin to all but last column.
&:not(:last-child) {
margin-right: $grid-size-large * 2;
}
Expand Down