Cover: Move cover min-height into dimensions panel via SlotFill #34065
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
aaronrobertshaw
added
[Type] Enhancement
7 tasks
|
Size Change: +73 B (0%) Total Size: 1.05 MB
ℹ️ View Unchanged
|
aaronrobertshaw
force-pushed
the
try/slot-fill-for-dimensions-panel
branch
from
ab8a037 to
6c0813e
Compare
aaronrobertshaw
requested review from
chrisvanpatten and
ellatrix
as code owners
aaronrobertshaw
force-pushed
the
update/move-cover-min-height-to-dimensions-panel
branch
from
ff6ca6f to
95cafdb
Compare
apeatling
reviewed
Aug 18, 2021
Contributor
There was a problem hiding this comment.
I went through and tested this:
✅ There should only be one "Dimensions" panel in the sidebar
✅ The min-height control should display by default under the "Dimensions" panel
✅ Confirm the min-height control continues to function correctly when adjust
❓ Confirm the min-height control behaves appropriately within the context of the ToolsPanel e.g. it can be reset etc.
When I reset all on the dimensions panel, the min height is reset, but the cover block stays at the same height it was set at. Is this expected? My expectation was it would reset to the default height when inserted. This may be the case on trunk so not a new thing.
✅ Ensure the min-height control does not appear in the Group block dimensions panel menu.
7 tasks
aaronrobertshaw
force-pushed
the
try/slot-fill-for-dimensions-panel
branch
2 times, most recently
from
cc7e089 to
3386a7d
Compare
aaronrobertshaw
force-pushed
the
update/move-cover-min-height-to-dimensions-panel
branch
from
95cafdb to
504dc9d
Compare
aaronrobertshaw
changed the base branch from
try/slot-fill-for-dimensions-panel
to
try/inspector-controls-with-toolspanel
Contributor
Author
|
Thanks for catching that @apeatling 👍 I wasn't able to replicate this at first until I started playing with the resizable box control. The Cover block's edit component does some odd handling of the min-height unit. I suspect this is a hangover from prior to the UnitControl offering a value inclusive of the CSS unit. Clearing the This PR has also been rebased onto an updated approach to injecting controls into the block support panels in #34157 |
aaronrobertshaw
force-pushed
the
try/inspector-controls-with-toolspanel
branch
from
515f33d to
852267f
Compare
aaronrobertshaw
force-pushed
the
update/move-cover-min-height-to-dimensions-panel
branch
from
e2f6cdd to
c103640
Compare
Contributor
|
The code change is looking good to me @aaronrobertshaw, and the testing steps all worked for me. I ran into a peculiar issue though clicking between two Cover blocks on the one post, though. I'm wondering if it's to do with selecting between the same kind of block so the ToolsPanel doesn't get unmounted? In the below video, when I click between two Cover blocks a couple of times, the top-most block loses all of the panel items and the panel item header. Clicking away from either cover block and then back to the top cover block restores it: dimensions-panel-switch-between-blocks.mp4 |
aaronrobertshaw
force-pushed
the
update/move-cover-min-height-to-dimensions-panel
branch
from
c103640 to
6efb585
Compare
Contributor
Author
|
Thanks for catching that @andrewserong! It turns out the deregistration of the panel items was occurring after the new panel item, matching the old one's label, was added. This resulted in them both being removed. This problem was addressed in b9c995a for the PR this is based on: #34157. The linked commit fixes the issue for now however there might be a more elegant solution we can iterate on in a future PR. Screen.Recording.2021-08-25.at.4.17.12.pm.mp4 |
Contributor
|
Thanks @aaronrobertshaw, that seems to have fixed it for me when clicking between a Group block and a Cover block. However, if I click between two Cover blocks, it still results in the top-most Cover block losing all its panel items: dimensions-panel-switch-between-two-cover-blocks.mp4I also noticed a strange thing when setting Margin on the Group block before clicking on the Cover block. If I haven't set Margin previously, clicking on the Cover block removes it from the Group block again. Since we haven't enabled Margin on the Group block in trunk, this is likely lower priority: dimensions-panel-switch-between-blocks-margin.mp4 |
aaronrobertshaw
force-pushed
the
try/inspector-controls-with-toolspanel
branch
from
b79628b to
4bc9c74
Compare
aaronrobertshaw
force-pushed
the
update/move-cover-min-height-to-dimensions-panel
branch
from
3707500 to
a875a76
Compare
7 tasks
aaronrobertshaw
force-pushed
the
try/inspector-controls-with-toolspanel
branch
from
4bc9c74 to
16d3c0a
Compare
aaronrobertshaw
force-pushed
the
update/move-cover-min-height-to-dimensions-panel
branch
from
a875a76 to
c9ce368
Compare
aaronrobertshaw
force-pushed
the
try/inspector-controls-with-toolspanel
branch
from
16d3c0a to
31dff94
Compare
aaronrobertshaw
force-pushed
the
update/move-cover-min-height-to-dimensions-panel
branch
2 times, most recently
from
2b82740 to
8fea6d9
Compare
This was referenced Sep 6, 2021
aaronrobertshaw
force-pushed
the
try/inspector-controls-with-toolspanel
branch
2 times, most recently
from
574c27b to
e9bbbed
Compare
aaronrobertshaw
force-pushed
the
update/move-cover-min-height-to-dimensions-panel
branch
from
8fea6d9 to
7660304
Compare
aaronrobertshaw
force-pushed
the
try/inspector-controls-with-toolspanel
branch
from
e9bbbed to
269b5d1
Compare
Base automatically changed from
try/inspector-controls-with-toolspanel
to
trunk
September 10, 2021 04:33
aaronrobertshaw
force-pushed
the
update/move-cover-min-height-to-dimensions-panel
branch
from
7660304 to
2ba3fe5
Compare
andrewserong
approved these changes
Sep 10, 2021
Contributor
andrewserong
left a comment
andrewserong
left a comment
There was a problem hiding this comment.
This is testing nicely @aaronrobertshaw!
✅ The min height control behaves as expected and is displayed in the Dimensions panel
✅ The onDeselect and resetAllFilter behaviour is working as expected
✅ Switching between group and cover blocks, or cover to cover blocks results in the Dimensions panel being updated as expected
I noticed one issue that's unrelated to this PR (it's happening for Group blocks too) and that's that if we copy a block with the Dimensions panel and then paste it elsewhere, the Dimensions panel is then empty. However if you click away from the block and back to it, it restores the behaviour. I don't think this is blocking for this PR, we can look into it separately, but here's a screenshot:
Contributor
Author
|
Thanks as always for the thorough testing and reviews @andrewserong. Agreed, let's create an issue for that and address it in an immediate follow up PR. |
aaronrobertshaw
deleted the
update/move-cover-min-height-to-dimensions-panel
branch
Contributor
|
Thanks Aaron! I've added a follow-up issue here: #34724 |
36 tasks
7 tasks
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.
Depends on: #34157
Related: #33970
Description
min-heightcontrol into the dimensions block support panel.ToolsPanelItemcomponent so it can also be reset via theToolsPanelmenu.How has this been tested?
Manually.
min-heightcontrol should display by default under the "Dimensions" panelmin-heightcontrol continues to function correctly when adjustmin-heightcontrol behaves appropriately within the context of theToolsPanele.g. it can be reset etc.min-heightcontrol does not appear in the Group block dimensions panel menu.Screenshots
Screen.Recording.2021-08-13.at.3.14.36.pm.mp4
Types of changes
Enhancement
Checklist:
*.native.jsfiles for terms that need renaming or removal).