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

Fix columns block style on the frontend #7128

Merged
merged 1 commit into from Jun 4, 2018

Conversation

Projects
None yet
4 participants
@jorgefilipecosta
Member

jorgefilipecosta commented Jun 4, 2018

Description

Fixes: #7127.

How has this been tested?

Use the columns block verify things work as expected on the editor, posts with columns block and see that on the front end the columns appear as expected.

@GlennMartin1

This comment has been minimized.

GlennMartin1 commented Jun 4, 2018

Pending approvals, perhaps this should be a point-release, i.e. 3.0.1?

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 4, 2018

Without the following, the columns on the front-end are arbitrarily sized:

	@for $i from 2 through 6 {
		&.has-#{ $i }-columns .editor-inner-blocks {
			grid-auto-columns: #{ 100% / $i };
		}
	}

The above sets the width to 33% if there are three columns. I think we need to output this separately for the front and the backend. Not sure how to do that.

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Jun 4, 2018

Hi @jasmussen,

Without the following, the columns on the front-end are arbitrarily sized:´

But I'm not removing the rule, just the .editor-inner-blocks part which I think is not needed.

@jasmussen jasmussen self-requested a review Jun 4, 2018

@jasmussen

Excellent work.

The problem was that .editor-inner-blocks did not exist on the front-end, but only on the back-end. Due to the need for nested grid column styles, we needed separate styles for the front and back-end.

This is tested locally and works on front-end and back-end with correctly sized columns. Nice work, thank you! 👍 👍

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Jun 4, 2018

Thank you for the review and for catching the previous problem with the widths that existed in this fix @jasmussen!

@for $i from 2 through 6 {
&.has-#{ $i }-columns .editor-inner-blocks {
&.has-#{ $i }-columns {

This comment has been minimized.

@aduth

aduth Jun 4, 2018

Member

This is a base stylesheet, meaning that in the editor, we have styles for both .has-x-columns and .has-x-columns .editor-inner-blocks to apply the grid-auto-columns. Will there be issues from this?

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 4, 2018

Member

I don't think we have a problem because the styles in the editor will be applied to .wp-block-columns and we set it to display block so my expectation is that grid styles are simply ignored.

This comment has been minimized.

@aduth

aduth Jun 4, 2018

Member

Makes me wonder a bit about nested nested blocks, Columns > Columns, where .editor-inner-blocks descendent could match multiple nodes. Probably outside the scope of this.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Jun 4, 2018

Member

I tested a little bit and it looks like it works even in nested columns. I think because this style &.has-#{ $i }-columns { that is .wp-block-columns.has-2-columns does not uses the hierarchy it just selects elements with both classes .wp-block-columns and .has-2-columns. If the styles of the editor were applied on the front we would have a problem but in this case, I think we are safe.

@jorgefilipecosta jorgefilipecosta merged commit a2520a3 into master Jun 4, 2018

2 checks passed

codecov/project 46.39% remains the same compared to 94889af
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jorgefilipecosta jorgefilipecosta deleted the fix/collumns-block-on-front-end branch Jun 4, 2018

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