Try: Fix regression with mover and block control sizes. #40037
Merged
Conversation
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
jasmussen
added
[Type] Bug
This was referenced Apr 5, 2022
|
Size Change: +46 B (0%) Total Size: 1.22 MB
ℹ️ View Unchanged
|
Mamaduka
approved these changes
Apr 5, 2022
Member
Mamaduka
left a comment
Mamaduka
left a comment
There was a problem hiding this comment.
Thanks for the quick follow-up, @jasmussen.
It works as expected.
I agree that refactoring is the better option, but probably not a good idea to do it a week before WP 6.0 beta.
I noticed that the spacing is a little off when the block name is displayed in the toolbar vs. when it's not. But this might not be related.
How to reproduce:
Add two Reusable blocks to the post. Lock block removal for one and compare toolbars.
Contributor
Author
|
Agreed on a small fix. Can reproduce the issue you mention. This branch: Trunk:
This is remarkably tricky to fix. It works in trunk exactly because the symbol and title exist in a toolbar group and get padding left and right. But to fix the majority of problems here, I had to zero out that same padding in block control toolbar groups, otherwise this would happen:
To an extent, I would consider the entire mover control, including drag handle and/or lock icon, to be a single toolbar group. That should — with the exception of the drag handle being 24px wide vs. 36px — make it all work well. But that's the refactor I think will take a little careful nudging. Let me grab a coffee and see if I can think of a quick bandaid. |
Member
|
The block title shouldn't be hidden when the removal is disabled. It's a different bug, but I think this still leaves us with spacing inconsistency. |
Contributor
Author
|
I think I fixed the spacing:
Yes, this is a separate PR right? |
Member
Correct. I've half working RR locally, just need to make CSS adjustments to the |
Contributor
Author
|
Look at it this way: it'll feel great to remove an ocean of code once we get a chance to refactor it! Thanks for the reviews, I'll land this one when the checks pass. |
Member
|
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?
Addresses feedback in #39920 (comment).
Changes there made the mover control and block locking icons too wide:
This PR, although slightly hacky, fixes it:
Why?
This is a quick bandaid. A better solution is to refactor the mover control to be a single toolbar group, so it can benefit from the very same simplified spacing metrics which were the cause of the regression. See also this codepen:
In the meantime, this might be a good bandaid. The code surrounding those controls is already a bit messy.
Testing Instructions
Test block toolbars, notably the mover control in a few case: