Post Featured Image: Move width and height controls into the Dimensions panel via SlotFill #36540
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
stacimc
added
[Status] In Progress
|
Size Change: -127 B (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
ramonjd
reviewed
Nov 16, 2021
packages/block-library/src/post-featured-image/dimension-controls.js
Outdated
Show resolved
Hide resolved
| { !! height && ( | ||
| <ToggleGroupControl | ||
| <ToolsPanelItem |
There was a problem hiding this comment.
I'm also seeing the vanishing controls:
I'm not 100% certain, but it seems to be related to the conditional rendering of the ToolsPanel.
When I remove the wrapper, things work okay:
{ !! height && (
<ToggleGroupControl
label={ scaleLabel }
value={ scale }
help={ scaleHelp[ scale ] }
onChange={ ( value ) =>
setAttributes( {
scale: value,
} )
}
isBlock
>
{ SCALE_OPTIONS }
</ToggleGroupControl>
) }Relocating the height condition also seems to work:
<ToolsPanelItem
...
>
{ !! height && (
<ToggleGroupControl
label={ scaleLabel }
value={ scale }
help={ scaleHelp[ scale ] }
onChange={ ( value ) =>
setAttributes( {
scale: value,
} )
}
isBlock
>
{ SCALE_OPTIONS }
</ToggleGroupControl>
) }
</ToolsPanelItem>Though relocating, and not removing ToolsPanelItem above causes a Scale option to the appear in the ToolsPanel options menu even where the control isn't shown.
Do we need the scale control to appear in the ToolsPanel options menu at all?
It's "toggleability"™ is determined only by the height value as far as I can tell.
Maybe we could reset the scale elsewhere, e.g., by detecting changes to the height if need be? 🤷
Ignore the above, the scale value resets just fine 😄
There was a problem hiding this comment.
Whoa, interesting find! Weird -- I know we have conditionally rendered ToolsPanelItems, like the way we only render enabled controls in the typography panel. Nice spot, I'll play around with this 💯
There was a problem hiding this comment.
It feels a bit off to remove the ToolsPanelItem wrapper, since then we'd have a control in the ToolsPanel which isn't tracked in the menu. I see your point here, though, since its reset is meant to be tightly linked to Height. Unfortunately it also seems to break some of the styling with the single-column height 🤔 If that's the way we go, I can fix that specifically for this block I think.
At the same time it also feels wrong to keep the ToolsPanelItem wrapper and only conditionally render the control for the reason you pointed out -- the option appears in the reset menu even when the control isn't shown.
There was a problem hiding this comment.
since then we'd have a control in the ToolsPanel which isn't tracked in the menu
Yeah, I see what you mean.
I suppose it depends on what folks expect ToolsPanel options menu items to represent.
For example, when controls are not shown by default, we can toggle their visibility/reset their values independent of any sibling control. At least as far as I know!
This wouldn't apply to the Scale control where there is no height value.
The merit I see in adding Scale to the menu is to indicate to users that resetting all controls in the panel will also reset the scale value (which appears to occur anyway).
But resetting all also makes the scale item disappear from the menu itself.
It does introduce a new paradigm. 🤔
|
Thanks @stacimc This is working fine for me, so long as I remove the I didn't go deep on the why, but @aaronrobertshaw will know. Even so, I'm not sure "Scale" even needs to be added to the options menu for things to work. What do you think? |
45 tasks
7 tasks
stacimc
force-pushed
the
update/move-featured-image-height-width-to-dimensions-panel
branch
from
d70be69
to
b5ab519
Compare
|
This has been updated, and now relies on some updates to the ToolsPanel to allow for conditionally displaying ToolsPanelItems; I've separated that out into #36588. To keep it as consistent as possible with the old behavior, the Scale control is now conditionally displayed only when a Height value is provided. This means it's also not included in the ToolsPanel menu as an optional control if there's no provided height. I think this is preferable, because we actively don't want the user to be able to adjust that control unless they've modified height. |
aaronrobertshaw
approved these changes
Nov 18, 2021
There was a problem hiding this comment.
Thanks for putting this together @stacimc. I appreciate the amount of effort that went into the fixes to the ToolsPanel to allow conditionally rendering default controls and facilitate the changes in this PR 🙇
After rebasing this on top of #36588, it tests really well for me. The code changes look good and I'm glad we can maintain the UX with the scale control linked to the height.
Once #36588 lands, I think this should be good to go.
Screen.Recording.2021-11-18.at.5.56.40.pm.mp4
stacimc
force-pushed
the
update/move-featured-image-height-width-to-dimensions-panel
branch
from
9108a22
to
f744a0d
Compare
|
The necessary ToolsPanel fixes in #36588 have been merged, and this PR rebased. It should now be ready for testing 😄 |
aaronrobertshaw
approved these changes
Nov 24, 2021
There was a problem hiding this comment.
After the rebase to pick up the latest changes from #36588 this tests well for me.
The failing e2es were all timeouts that look unrelated. I've taken the liberty of re-running those failed tests. Once they pass, this should be ok to merge.
stacimc
deleted the
update/move-featured-image-height-width-to-dimensions-panel
branch
|
@noisysocks It looks like we forgot to backport this into 5.9 betas. So I'm tentatively adding the "backport to the minor" label. |
Mamaduka
added
the
Backport to WP Minor Release
This was referenced Jan 12, 2022
|
Cherry-picked for 5.9.1 |
Mamaduka
removed
the
Backport to WP Minor Release
Mamaduka
pushed a commit
that referenced
this pull request
Feb 16, 2022
…ns panel via SlotFill (#36540) * Move width and height controls into the Dimensions panel via SlotFill * Add min value & correctly reset scale * Show Scale as default when rendered * Only allow resetting scale when it is not the default value * Do not reset scale when height is reset
27 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.
Fixes #36536
Description
The Post Featured Image block had its Height/Width/Scale controls in its own Dimensions panel. With the addition of the new Dimensions ToolsPanel, this meant we had two panels with the same name. This PR moves the custom height/width/scale controls into the Dimensions ToolsPanel via the SlotFill.
Because it conditionally displays the Scale control, it depends on #36588 which fixes some bugs with panel item registration and de-registration.Updated: necessary fixes have been mergedHow has this been tested?
Important: rebase on #36588 to get some ToolsPanel fixes.Some things to call out in particular:
The scale control is reset to its default 'cover' value when the height is resetFor consistency with the way the controls used to work, Scale is not reset when Height is reset:Screenshots
Video:
featured-post-image.mov
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).