Tune appender margin. #33866
Merged
Tune appender margin. #33866
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +15 B (0%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
Aug 4, 2021
| @@ -16,7 +16,7 @@ | |||
|
|
|||
| // Add a uniform margin around the block. | |||
| // This can hopefully avoid havoc in flex containers. | |||
| margin: $grid-unit-10; | |||
| margin: 0 $grid-unit-10; | |||
There was a problem hiding this comment.
I actually think that margin should be removed entirely, it's not the appender's decision to space itself to its adjacent blocks, it should be the responsibility of its container layout. (think gap)
I actually thought I have removed this margin when I introduced the "flex" layout. Maybe I missed some places?
There was a problem hiding this comment.
One place you could see this margin fail today is:
- Insert a columns block,
- Insert an image placeholder in the first column
- Notice the appender after the placeholder, it should be left aligned by the edge of the columns block without any margin.
Conceptually speaking, we should be working on removing the responsibility to position the appender from the appender component itself and move into its layout (containers)
There was a problem hiding this comment.
Conceptually speaking, we should be working on removing the responsibility to position the appender from the appender component itself and move into its layout (containers)
Good point. I'll take a stab. Thanks Riad!
There was a problem hiding this comment.
To a little surprise for me, it's working entirely fine removing that margin:
I wasn't able to remove the margin entirely, I had to zero out a bottom margin in 62935f2 which is inherited from common.css — this one:
There was a problem hiding this comment.
This works well yes. I think there's still a small issue with the Navigation block when there's only a single menu item, the gap is not applied. Ideally the block should be refactored to use the flex layout (which we'll probably do later) but in the meantime if you want to add (or remove) some specific styles to that block to handle that, that's fine.
There was a problem hiding this comment.
You'll be happy to know that it actually does use flex:
... and I have this one handy to move from using margins to using gap: #32367
This works well yes. I think there's still a small issue with the Navigation block when there's only a single menu item, the gap is not applied.
Are you referring to this state:
or this state?
In the second case, it's accurate, as when the navigation block doesn't have a background color, individual menu items also don't have any padding applied. That was done in #30805 to enable more patterns. You can still force a padding using global styles, and hopefully in the future an inspector control, but as far as defaults go, menu items are paddingless by default.
There was a problem hiding this comment.
I was referring to the last one and in my test I didn't see any gap between "Properties" and the appender. Might be a theme specific issue (2021)
There was a problem hiding this comment.
Yep, 2021 styles an older version of the navigation block heavily to the point that global styles don't work on it either:
I've opened https://core.trac.wordpress.org/ticket/53220 about that. Thanks again.
youknowriad
approved these changes
Aug 4, 2021
|
Thanks for the review. I'll land this one when I get the tests to pass! |
|
The test failure here was intermittent, I opened #33875 about it. |
annezazu
added
[Block] Navigation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Followup to #33739. There was a teeeeensy side effect of the uniform 8px margin around the appender. Specifically that it in some cases made the height of blocks larger. See here how the navigation block grows 4px taller on select:
That's because the 24px image plus 2x8px margins comes to 40px, but in this theme the menu is 36px tall by default.
This PR mitigates that by removing the top/bottom margin:
I say mitigates because if the menu is <24px tall the appender would still make it bigger. That's a separate and bigger issue to solve (see conversation in #31886). But it still seems like a good, small improvement. The appender is still vertically centered due to the flex rules.
How has this been tested?
Insert social links, buttons, navigation blocks, and verify the appender on select still looks good.
Checklist:
*.native.jsfiles for terms that need renaming or removal).