Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Post Featured Image: Move width and height controls into the Dimensions panel via SlotFill #36540

Merged
merged 5 commits into from
Nov 24, 2021
Merged

Post Featured Image: Move width and height controls into the Dimensions panel via SlotFill #36540

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
55c19b7
Move width and height controls into the Dimensions panel via SlotFill
stacimc Nov 16, 2021
5b75caf
Add min value & correctly reset scale
stacimc Nov 16, 2021
735b976
Show Scale as default when rendered
stacimc Nov 17, 2021
7bbc003
Only allow resetting scale when it is not the default value
stacimc Nov 17, 2021
f744a0d
Do not reset scale when height is reset
stacimc Nov 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/block-editor/src/hooks/dimensions.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
.dimensions-block-support-panel {
.single-column {
grid-column: span 1;
}
}
1 change: 1 addition & 0 deletions packages/block-editor/src/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
@import "./hooks/anchor.scss";
@import "./hooks/layout.scss";
@import "./hooks/border.scss";
@import "./hooks/dimensions.scss";
@import "./hooks/typography.scss";

@import "./components/block-toolbar/style.scss";
Expand Down
125 changes: 75 additions & 50 deletions packages/block-library/src/post-featured-image/dimension-controls.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,15 @@
/**
* External dependencies
*/
import classNames from 'classnames';

/**
* WordPress dependencies
*/
import { __, _x } from '@wordpress/i18n';
import {
PanelBody,
__experimentalUnitControl as UnitControl,
Flex,
FlexItem,
__experimentalToggleGroupControl as ToggleGroupControl,
__experimentalToggleGroupControlOption as ToggleGroupControlOption,
__experimentalUseCustomUnits as useCustomUnits,
__experimentalToolsPanelItem as ToolsPanelItem,
} from '@wordpress/components';
import { useSetting } from '@wordpress/block-editor';
import { InspectorControls, useSetting } from '@wordpress/block-editor';

const SCALE_OPTIONS = (
<>
Expand All @@ -38,6 +31,8 @@ const SCALE_OPTIONS = (
</>
);

const DEFAULT_SCALE = 'cover';

const scaleHelp = {
cover: __(
'Image is scaled and cropped to fill the entire space without being distorted.'
Expand All @@ -51,6 +46,7 @@ const scaleHelp = {
};

const DimensionControls = ( {
clientId,
attributes: { width, height, scale },
setAttributes,
} ) => {
Expand All @@ -72,53 +68,82 @@ const DimensionControls = ( {
};
const scaleLabel = _x( 'Scale', 'Image scaling options' );
return (
<PanelBody title={ __( 'Dimensions' ) }>
<Flex
justify="space-between"
className={ classNames(
'block-library-post-featured-image-dimension-controls',
{ 'scale-control-is-visible': !! height }
) }
<InspectorControls __experimentalGroup="dimensions">
<ToolsPanelItem
className="single-column"
hasValue={ () => !! height }
label={ __( 'Height' ) }
onDeselect={ () => setAttributes( { height: undefined } ) }
resetAllFilter={ () => ( {
height: undefined,
} ) }
isShownByDefault={ true }
panelId={ clientId }
>
<UnitControl
label={ __( 'Height' ) }
labelPosition="top"
value={ height || '' }
min={ 0 }
onChange={ ( nextHeight ) =>
onDimensionChange( 'height', nextHeight )
}
units={ units }
/>
</ToolsPanelItem>
<ToolsPanelItem
className="single-column"
hasValue={ () => !! width }
label={ __( 'Width' ) }
onDeselect={ () => setAttributes( { width: undefined } ) }
resetAllFilter={ () => ( {
width: undefined,
} ) }
isShownByDefault={ true }
panelId={ clientId }
>
<FlexItem>
<UnitControl
label={ __( 'Height' ) }
labelPosition="top"
value={ height || '' }
onChange={ ( nextHeight ) => {
onDimensionChange( 'height', nextHeight );
} }
units={ units }
/>
</FlexItem>
<FlexItem>
<UnitControl
label={ __( 'Width' ) }
labelPosition="top"
value={ width || '' }
onChange={ ( nextWidth ) => {
onDimensionChange( 'width', nextWidth );
} }
units={ units }
/>
</FlexItem>
</Flex>
<UnitControl
label={ __( 'Width' ) }
labelPosition="top"
value={ width || '' }
min={ 0 }
onChange={ ( nextWidth ) =>
onDimensionChange( 'width', nextWidth )
}
units={ units }
stacimc marked this conversation as resolved.
Show resolved Hide resolved
/>
</ToolsPanelItem>
{ !! height && (
<ToggleGroupControl
<ToolsPanelItem
Copy link
Member

@ramonjd ramonjd Nov 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also seeing the vanishing controls:

Nov-17-2021 08-10-44

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 😄

Nov-17-2021 08-23-51

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 💯

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@ramonjd ramonjd Nov 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 🤔

hasValue={ () => !! scale && scale !== DEFAULT_SCALE }
label={ scaleLabel }
value={ scale }
help={ scaleHelp[ scale ] }
onChange={ ( value ) => {
onDeselect={ () =>
setAttributes( {
scale: value,
} );
} }
isBlock
scale: DEFAULT_SCALE,
} )
}
resetAllFilter={ () => ( {
scale: DEFAULT_SCALE,
} ) }
isShownByDefault={ true }
panelId={ clientId }
>
{ SCALE_OPTIONS }
</ToggleGroupControl>
<ToggleGroupControl
label={ scaleLabel }
value={ scale }
help={ scaleHelp[ scale ] }
onChange={ ( value ) =>
setAttributes( {
scale: value,
} )
}
isBlock
>
{ SCALE_OPTIONS }
</ToggleGroupControl>
</ToolsPanelItem>
) }
</PanelBody>
</InspectorControls>
);
};

Expand Down
10 changes: 6 additions & 4 deletions packages/block-library/src/post-featured-image/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ const placeholderChip = (
);

function PostFeaturedImageDisplay( {
clientId,
attributes,
setAttributes,
context: { postId, postType, queryId },
Expand Down Expand Up @@ -137,11 +138,12 @@ function PostFeaturedImageDisplay( {

return (
<>
<DimensionControls
clientId={ clientId }
attributes={ attributes }
setAttributes={ setAttributes }
/>
<InspectorControls>
<DimensionControls
attributes={ attributes }
setAttributes={ setAttributes }
/>
<PanelBody title={ __( 'Link settings' ) }>
<ToggleControl
label={ sprintf(
Expand Down
8 changes: 0 additions & 8 deletions packages/block-library/src/post-featured-image/editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,11 +128,3 @@ div[data-type="core/post-featured-image"] {
display: block;
}
}

.block-library-post-featured-image-dimension-controls {
margin-bottom: $grid-unit-10;

&.scale-control-is-visible {
margin-bottom: $grid-unit-20;
}
}