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

FSE: Update FSE editor css to remove template-block space #35070

Closed
wants to merge 1 commit into from

Conversation

apeatling
Copy link
Member

@apeatling apeatling commented Aug 1, 2019

Changes proposed in this Pull Request

This removes the spacing above template part blocks.

Before:

before

After:

after

Testing instructions

  • Create a new page or edit an existing page and confirm the space before the header template block has been removed.
  • Edit the header template and confirm spacing here still works correctly.
  • Create or edit a post and confirm editor spacing is not affected.

Fixes: #34890

@apeatling apeatling added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Aug 1, 2019
@apeatling apeatling requested a review from glendaviesnz August 1, 2019 23:09
@apeatling apeatling requested review from a team as code owners August 1, 2019 23:09
@apeatling apeatling self-assigned this Aug 1, 2019
@matticbot
Copy link
Contributor

@apeatling apeatling removed the request for review from a team August 1, 2019 23:10
}

.template-block {
margin-top: -28px;
Copy link
Member Author

Choose a reason for hiding this comment

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

Since we can't target parent selectors accurately, the only way I can see to remove the editor UI spacing is to use a negative margin here. The 28px space is there for editor block insertion UI (the (+) button before blocks) which is never present for these template blocks.

Copy link
Contributor

Choose a reason for hiding this comment

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

This does also make the footer slightly overlap the the blocks above it when I apply this locally:

footer

... just looking to see if there is a way to easily target the header template separately

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes, I think we can actually remove this negative margin hack by adding top level css classes like here: https://github.com/Automattic/wp-calypso/pull/35081/files#diff-d4f340da4f72898d36eb7f2ca6c1e127R31

Copy link
Contributor

Choose a reason for hiding this comment

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

Wild idea, why not applying the negative margin too to the wrapper?

.editor-styles-wrapper {
	margin-top: -28px;
	padding: 0;
}

(AFAICS the wrapper doesn't really need margin: 0, but maybe I'm not testing some cases 🤔 )

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

margin: 0;
}

.template-block {
Copy link
Contributor

@glendaviesnz glendaviesnz Aug 2, 2019

Choose a reason for hiding this comment

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

The following will target just the header template block:

Suggested change
.template-block {
.editor-block-list__block:first-child {
.template-block {
margin-top: -28px;
}
}

But seems a bit fragile - I wonder it we need to try and get some custom classes in the header and footer blocks in order to more reliably target these?

@noahtallen
Copy link
Contributor

noahtallen commented Aug 2, 2019

I wonder if it would be more straightforward to get the outer "frame" color working? #34684 It filled in the whitespace in a nice way and it seems like we wouldn't have to do a ton to get it working!

@apeatling
Copy link
Member Author

This one is on hold until #35115 is merged, since it will use the new top level class names for our template blocks.

@apeatling
Copy link
Member Author

I wonder if it would be more straightforward to get the outer "frame" color working?

This helps, but there is still extra space even with that frame. You can try #35116 to see this.

@apeatling
Copy link
Member Author

Going to close this one and refactor and open a new PR.

@apeatling apeatling closed this Aug 6, 2019
@vindl vindl deleted the update/fse-template-block-spacing branch August 6, 2019 02:45
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Aug 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FSE: Header blocks are rendered with different spacing on the frontend
5 participants