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

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

Merged
merged 9 commits into from
Feb 1, 2019
Merged

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

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
89a9859
Remove unintentional right-margin on last odd-item.
m-e-h Nov 21, 2018
ce9ff9a
update editor to match style.css column margin changes
m-e-h Dec 12, 2018
3930c4a
account for beginning and end margins with breakpoints
m-e-h Dec 13, 2018
b28e9af
update editor.css to match style.css
m-e-h Dec 13, 2018
ace6ef4
account for editor block padding and the medium breakpoint
m-e-h Dec 13, 2018
3b812ba
Merge branch 'master' into fix/column-margins
m-e-h Jan 29, 2019
66ed077
remove whitespace
m-e-h Jan 29, 2019
e8acfd0
account for negative margins added by the editor
m-e-h Jan 29, 2019
7f5fe9d
correct flex-basis calculation for column width
m-e-h Jan 30, 2019
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
33 changes: 14 additions & 19 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 @@ -10,41 +10,36 @@
}

.wp-block-column {
flex: 1;
flex-grow: 1;
Copy link

Choose a reason for hiding this comment

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

I don't think this property is necessary since we are setting the width to 100% below.

margin-bottom: 1em;

// Responsiveness: Show at most one columns on mobile.
flex-basis: 100%;
Copy link

Choose a reason for hiding this comment

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

If we change this to width:100%; we accomplish the same thing and make it easier to override.

(content < width < flex-basis)


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

// 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.

// 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.
@include break-small() {
&:nth-child(odd) {
margin-right: $grid-size-large * 2;
}
&:nth-child(even) {
margin-left: $grid-size-large * 2;
}

&:not(:first-child) {
margin-left: $grid-size-large * 2;
// Beyond mobile, allow 2 columns.
flex-basis: 50%;
Copy link

Choose a reason for hiding this comment

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

If we're getting rid of the 2-column breakpoint, then flex-basis:auto; would make more sense here wouldn't it?

flex-grow: 0;
Copy link

Choose a reason for hiding this comment

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

flex-grow:0; isn't necessary here, if we remove flex-grow:1; above.


// 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.
margin-right: $grid-size-large * 2;
Copy link
Member

Choose a reason for hiding this comment

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

These seem like a generally much easier set of rules to follow. I kept thinking I'd have thought this would have the effect of doubling up the margin from what had existed previously since only one or the other horizontal margin was applied, but I realized after some time I'd totally missed the previous &:not(:last-child) { rule.

Copy link

Choose a reason for hiding this comment

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

Using &:not(:last-child) and &:not(:first-child) requires fewer lines of code, but it's admittedly a little confusing. I think it just depends on what your priorities are. Both do the same thing.

margin-left: $grid-size-large * 2;

&:first-child {
margin-left: 0;
}

&:not(:last-child) {
margin-right: $grid-size-large * 2;
&:last-child {
margin-right: 0;
}
}
}