Fix block toolbar height and rounded corners of parent selector button when text label mode #49556
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
t-hamano
added
[Feature] Blocks
|
Size Change: -7 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 80b1459. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4676863099
|
|
Thanks for working on this, it seems to fix the original issue. One small detail I spotted that would also be nice to fix – the parent selector has a border-radius on the right-hand corners which looks a bit broken:
|
|
Good catch! I have fixed that problem as well. This change should not affect the parent selector in default mode.
|
t-hamano
changed the title
|
Apologies, I should have mentioned this yesterday (and perhaps it's the reason for the rounded corners in the first place), should the button be detached from the toolbar, like it is when text labels are off? It's probably a separate point, but I also wonder about the label. "Select |
|
The main purpose of this PR is to eliminate 1px misalignment. I think the label and position of the parent select button may be better discussed in another issue. |
|
Thanks for the attention to detail. Agreed we can make small fixes and move forward. The truth is the "labels only" interface at the moment feels like it could benefit from a dedicated issue with a design. Noted across a couple of separate threads, the issue is that some of the labels need rephrasing when they are label only, and font size definitely needs to be considered. I like how the MacOS finder balances this:
Again, all that's separate! Thanks for the work and the reviews! |
Fair enough! I'm not a huge fan of using negative <1px margins. It feels like it could be a bit fragile? Did you consider simply reducing the height of the move up/down buttons by 1px? That seems to address the issue as well. Are there other options? |
|
I have considered three other approaches to this approach: Change the height of the "Move up" button to 23pxThe "Move down" button is still 24px high, so the sizes do not match.
Change the height of "Move up" and "Move down" buttons to 23.5pxA decimal point must be used, but no negative values are used.
Absolute placement of the separator lineMargin with decimal points is needed to center the display completely at the top and bottom. The number of properties seems a bit large.
Overall, I chose the simplest approach with the least amount of code. What do you think? |
|
I suppose another option would be to remove the separator... we don't have one in icon mode.
|
t-hamano
force-pushed
the
fix/block-toolbar-height
branch
from
573936b to
3433047
Compare
Surely, that makes sense! I removed the border and it looks very natural. de8e0143bc44d321f11e9bd56f78b5c9.mp4 |
|
I think that works well. I pushed a small padding adjustment to move the labels away from the edge of the toolbar to reduce tension there. What do you think?
Edit: Nevermind, I noticed this messes up the horizontal movers, and there's no way to target them specifically. |
t-hamano
force-pushed
the
fix/block-toolbar-height
branch
from
80b1459 to
d36662e
Compare
|
@jameskoster |
jameskoster
approved these changes
Apr 17, 2023
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.
What?
This PR unifies the height of the block toolbar to 46px. This eliminates the misalignment of the buttons for selecting the parent block when "Show button text labels" mode is enabled.
edit: This PR also fixes the rounded corners on the parent selector button as pointed out in this comment.
Why?
The block toolbar is expected to be 46px high:
However, when "Show button text labels" mode is enabled, the toolbar height is 47px:
This is because the pseudo-element between "Move up" and "Move down" has a height of 1px:
This results in the height of the button to select the parent block being displaced if that block is a child:
How?
Since this problem can be solved by treating a 1px height border as having no height, there are many possible approaches. I have applied a total negative margin of 1px on the top and bottom, which seems the simplest.
Screenshots or screencast
Before
After