Fix the block toolbar styling when the 'Show button text labels' preference is enabled#44779
Merged
afercia merged 3 commits intoOct 12, 2022
Merged
Conversation
|
Size Change: +2.24 kB (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
jasmussen
approved these changes
Oct 10, 2022
jasmussen
left a comment
Contributor
There was a problem hiding this comment.
Thanks for the PR. Toolbar before, with icons:
... and with labels:
After, the toolbar with icons shows no change (because this is scoped to just the labels preference):
... and with labels:
As such, the PR fixes the badly cropped mover buttons.
I left some comments for potential simplification, but this addresses the issue.
| } | ||
|
|
||
| .block-editor-block-contextual-toolbar.is-fixed .block-editor-block-switcher { | ||
| border-right-color: $gray-300; |
Contributor
There was a problem hiding this comment.
Should be $gray-200 to match the other borders.
| } | ||
|
|
||
| .block-editor-block-mover__move-button-container { | ||
| padding-left: 6px; |
Contributor
There was a problem hiding this comment.
It looks like this can be simplified a fair bit, and reduced. Right now there's 6px padding left and right inside the button, and the overall container, providing 12px padding total. Since space is at a premium, this might benefit from a small refactor, so it can become just 6px left and right, like so:
Here's a quick inspecting session, a calc-based width needs updating for this to work:
| display: block; | ||
| height: 1px; | ||
| background: $gray-900; | ||
| width: calc(100% + 12px); |
Contributor
There was a problem hiding this comment.
This is the calc based width I referred to before. If you make that just 100%, and a 0px margin rather than negative margin, the move buttons can have reduced horizontal padding.
Member
Author
|
@jasmussen thanks for your feedback. I'm not sure we can set the right padding to 0 on the toolbar group, as that would apply to all the toolbar groups ending up in uneven padding. From your GIF:
I tried to fix it in a different way. I noticed one more issue. When there's only one block, the mover isn't shown. That makes sense but the styling is broken, as a redundant border and some padding stay there. This happens also on trunk. Screenshot:
I fixed it by setting the border on the mover instead of the switcher. Final result with all buttons shown:
|
Contributor
|
Since the changes are scoped to the labels only mode, and I like to think we can do a great deal more to improve that, seems fine to land this as a step in the right direction. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
What?
When the 'Show button text labels' preference is enabled, the block toolbar styling is broken:
Why?
How?
.show-icon-labels.orderwhich is generally not recommended for accessibility. However, in this case it only changes the position of the CSS pseudo element used as border below the 'Move up' button so it doesn't affect the order of the content. The buttons order is unchanged.Testing Instructions
Screenshots or screencast
Before:
After: