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

Fixes #7818 - added side padding to columns on front-end #9936

Closed
wants to merge 1 commit into from

Conversation

m-muhsin
Copy link

Description

Added padding-left: 5px and padding-right: 5px to .wp-block-column within .wp-block-columns
This will fix issue #7818

How has this been tested?

Tested on Twenty Seventeen theme on Windows 10 using Apache.
It probably will not affect other places since I added the code within .wp-block-columns .wp-block-column { } selector.

Screenshots

Before:
image

After:
image

Types of changes

Enhancement/Bug fix

@aduth aduth added Needs Design Feedback Needs general design feedback. [Feature] Nested / Inner Blocks Anything related to the experience of nested/inner blocks inside a larger container, like Group or P labels Sep 17, 2018
@GlennMartin1
Copy link

Could this be merged into 4.0?

@jasmussen
Copy link
Contributor

This seems good.

Could you replace the 5px with $grid-size-small instead? 5px seems a little arbitrary, even if it means 10px side by side. We're trying to introduce a 8px based grid in Gutenberg, so it'd be nice to use one of those units from https://github.com/WordPress/gutenberg/blob/master/edit-post/assets/stylesheets/_variables.scss#L19.

Going to expand the review range, and see if we can get this in.

@jasmussen jasmussen requested a review from a team October 2, 2018 19:59
@tofumatt
Copy link
Member

tofumatt commented Oct 2, 2018

@jasmussen: If 5px is changed to $grid-size-small, are you cool from a design perspective?

@jasmussen
Copy link
Contributor

Hmm, thinking a bit more about this; in principle yes because the margin is arguably structural as this is a new block, and it's good to use the grid units.

However we also want the front-end to not differ from the back-end in terms of visuals, and this patch would add padding left and right of each column, which means text inside a column would be indented compared to text that came before the column.

Additionally it seems like there isn't actually any margins between the two columns in the editor either — the space that sits between the two columns APPEARS to be coming from this:

.wp-block-columns .editor-block-list__layout:last-child {
    margin-right: -14px;
}

.wp-block-columns .editor-block-list__layout:first-child {
    margin-left: -14px;
}

Those 14px are from the $block-padding variable, and it appears those rules were added exactly to solve the problem of aligning with content before and after the columns content.

In other words it appears the editor view is buggy, and the frontend view is "correct".

So the better solution, it seems, would be to find a new unified way of adding space between the two columns — @youknowriad isn't there a flex-spacing property we can use? — and in addition to this, finding a better way in the editor and perhaps frontend both, to align the contents of the column with content before and after the columns block. Some deeper thought, in other words. I will think about this for sure, possibly try a patch.

Copy link
Member

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

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

In that case, I'm going to flag this as something that still needs design feedback and we won't be accepting as-is. Sorry about that for those who wanted to see this get merged, but it looks like it needs a bit more thought/work.

@chrisvanpatten
Copy link
Member

chrisvanpatten commented Oct 2, 2018

I had seen this ticket come up but hadn't had a chance to comment.

Just want to say after I spent about a few hours tonight trying to get custom nested block styles in sync between Gutenberg and the front-end, I would strongly be in favour of an improvement that would simplify the in-editor nested styles and make it possible to use a unified style between g7g and the front-end. I understand that until Shadow DOM is widely available, the complex layering of container elements, negative margins, etc are all necessary but it seems like we should be able to find an approach that simplifies it at least somewhat.

I am willing to cancel any and all meetings, client engagements, meals, etc. to find time to brainstorm/talk through this as well ;-)

@m-muhsin
Copy link
Author

m-muhsin commented Oct 3, 2018

Thanks for the initial feedback everyone. I'm willing to make a pull request again once the final feedback comes through. Cheers!

@karmatosed
Copy link
Member

Removing design feedback as we have it now and a way forward. Thanks everyone.

@karmatosed karmatosed removed the Needs Design Feedback Needs general design feedback. label Oct 9, 2018
@jasmussen
Copy link
Contributor

I have included your fix in #10541, so closing this one. Thanks again!

@jasmussen jasmussen closed this Oct 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Nested / Inner Blocks Anything related to the experience of nested/inner blocks inside a larger container, like Group or P
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants