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

Jump to bottom

Override our global aligncenter CSS rules for the cover block #11990

Merged
merged 2 commits into from
Nov 16, 2018
Merged

Override our global aligncenter CSS rules for the cover block #11990

merged 2 commits into from
Nov 16, 2018

Conversation

kjellr
Copy link
Contributor

@kjellr kjellr commented Nov 16, 2018

Previously, center-aligned cover blocks were ignoring our intended display: flex; and margin rules on the front-end. This ensures they appear as intended.

Before:

screen shot 2018-11-16 at 11 31 56 am

screen shot 2018-11-16 at 11 45 57 am

After:

screen shot 2018-11-16 at 11 42 32 am

screen shot 2018-11-16 at 11 44 17 am

This was discovered as part of WordPress/twentynineteen#612, but effects all themes.

@kjellr
Override our global aligncenter CSS rules for the cover block
5e85f67 
Previously, center-aligned cover blocks were ignoring our intended display and margin rules. This ensures they appear as intended.
@kjellr kjellr added [Type] Bug An existing feature does not function as intended [Block] Cover Affects the Cover Block - used to display content laid over a background image labels Nov 16, 2018
@kjellr kjellr requested a review from jasmussen November 16, 2018 16:46
jasmussen
jasmussen approved these changes Nov 16, 2018
View reviewed changes
Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

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

Code looks good but haven't tested. I can't quite grok what the margin rules do, but it seems good. Be sure to test all alignments. 👍👍

@kjellr
Copy link
Contributor Author

kjellr commented Nov 16, 2018 •
edited
Loading

I can't quite grok what the margin rules do, but it seems good.

Yeah, the margins are pretty harmless after all. I'll leave them out.

Be sure to test all alignments.

Ah, good call. This had to be expanded to the left/right alignments too:

Before:
screen shot 2018-11-16 at 12 06 49 pm

After:
screen shot 2018-11-16 at 12 07 31 pm

I took care of all that in 80efac3, but I'll hold off on merging until someone gets a chance to test just to be safe.

@kjellr kjellr requested a review from a team November 16, 2018 17:12
tofumatt
tofumatt approved these changes Nov 16, 2018
View reviewed changes
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.

Works for me.

@tofumatt tofumatt added this to the 4.5 milestone Nov 16, 2018
@kjellr kjellr merged commit 745eda1 into master Nov 16, 2018
@kjellr
Copy link
Contributor Author

kjellr commented Nov 16, 2018

Thanks @jasmussen and @tofumatt!

@kjellr kjellr deleted the fix/aligncenter-cover-block-css branch November 16, 2018 17:24
This was referenced Nov 16, 2018
Merged
Closed
@m-e-h m-e-h mentioned this pull request Nov 16, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasmussen jasmussen jasmussen approved these changes

@tofumatt tofumatt tofumatt approved these changes

Assignees
No one assigned
Labels
[Block] Cover Affects the Cover Block - used to display content laid over a background image [Type] Bug An existing feature does not function as intended
Projects
None yet
Milestone
4.5
Development

Successfully merging this pull request may close these issues.
3 participants
@kjellr @tofumatt @jasmussen