-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 site editor layout regressions #58077
Conversation
Size Change: -16 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
Flaky tests detected in 72af9ae64ef7dccfc4126a1ecdee8c1c16afa053. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7614231561
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR makes it appear to be working as expected. As an improvement idea, is it possible to delete duplicate styles between .edit-site-layout__area
and .edit-site-page
?
The approach I think is to apply to each element only the CSS expected for its role. From what I've tested, the following changes in this PR should also work:
.edit-site-layout__area {
flex-grow: 1;
margin: 0;
overflow: hidden;
@include break-medium() {
border-radius: 8px;
margin: $canvas-padding $canvas-padding $canvas-padding 0;
}
}
.edit-site-page {
color: $gray-800;
background: $white;
height: 100%;
}
I also found that disabling "New Admin View" created unnecessary margin on the Pattern page. This is because border-radius and margin are applied to .edit-site-layout__area
, but these two styles are required for this PR for now. In the old Patterns pages, we might need to override this style or remove the .edit-site-layout__area
element.
Absolutely. I'll push those changes shortly. I noticed the patterns issue too and left a note on the issue. I think that's one to address in a separate PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 The unit test failure seems to be fixed in the latest trunk. So if you merge the latest trunk into this PR, the test should pass.
72af9ae
to
af3746f
Compare
@jameskoster @ntsekouras Can we merge this PR? From what I've tested, it seems perfectly fine. |
Let's merge. I'll update the original issue to ensure all the other problems don't get lost. Edit: I saw you already updated the issue, thanks :D |
Fix some of the regressions introduced in #57938, as noted in #58044. Specifically:
Before
After