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

Min Height: Add height control component with slider #45875

Merged
merged 4 commits into from
Nov 24, 2022
Merged

Min Height: Add height control component with slider #45875

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c408c62
Min Height: Add height control component with slider
andrewserong Nov 18, 2022
d2aac5e
Fix typo for rem unit
andrewserong Nov 21, 2022
c8351eb
Add some handling for switching between unit types
andrewserong Nov 22, 2022
8cad9c7
Add storybook example
andrewserong Nov 22, 2022
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
94 changes: 94 additions & 0 deletions packages/block-editor/src/components/height-control/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
/**
* WordPress dependencies
*/
import { useMemo } from '@wordpress/element';
import {
BaseControl,
RangeControl,
Flex,
FlexItem,
__experimentalSpacer as Spacer,
__experimentalUseCustomUnits as useCustomUnits,
__experimentalUnitControl as UnitControl,
__experimentalParseQuantityAndUnitFromRawValue as parseQuantityAndUnitFromRawValue,
} from '@wordpress/components';
import { __ } from '@wordpress/i18n';

/**
* Internal dependencies
*/
import useSetting from '../use-setting';

const CUSTOM_VALUE_SETTINGS = {
px: { max: 1000, steps: 1 },
'%': { max: 100, steps: 1 },
vw: { max: 100, steps: 1 },
vh: { max: 100, steps: 1 },
em: { max: 50, steps: 0.1 },
rem: { max: 50, steps: 0.1 },
Copy link
Contributor

Choose a reason for hiding this comment

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

Was it a deliberate choice to increase the step value for em and rem values from other controls (0.01)? The PR description explains the higher max values only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, this one was also a copy + paste, but the intention of these custom step values is for it to only apply to the range control instead of the unit control. With the unit control it's important that the step value is small enough to support specific values that folks might use (so .25 is a useful fraction of an em or rem value for example). However, for the range control, I think it's valuable to use a slightly larger step value for the case of a user with the slider thumb focused pressing left/right keys. I've updated the PR description to include the following:

Note: higher step values are also included for the range control only. This allows pressing left/right arrows with the range control focused to have a more immediate visual effect on the minimum height in use. The higher values are only applied to the range control so that the unit control can still support more granular / precise values.

I've also renamed the constant so that it's hopefully a bit clearer that it only applies to the range control. Also, happy to go with different defaults if this feels off 🙂

};

export default function HeightControl( {
onChange,
label = __( 'Height' ),
value,
} ) {
const customRangeValue = parseFloat( value );

const units = useCustomUnits( {
availableUnits: useSetting( 'spacing.units' ) || [
'%',
'px',
'em',
'rem',
'vh',
'vw',
],
} );

const selectedUnit =
useMemo(
() => parseQuantityAndUnitFromRawValue( value ),
[ value ]
)[ 1 ] || units[ 0 ].value;
Copy link
Contributor

Choose a reason for hiding this comment

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

From memory, I think it's possible we can get an empty units array. Should we guard against that edge case here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, I've added a fallback px to this line.


const handleSliderChange = ( next ) => {
onChange( [ next, selectedUnit ].join( '' ) );
};

return (
<fieldset className="component-height-control">
<BaseControl.VisualLabel as="legend">
{ label }
</BaseControl.VisualLabel>
<Flex>
<FlexItem isBlock>
<UnitControl
value={ value }
units={ units }
onChange={ onChange }
min={ 0 }
size={ '__unstable-large' }
/>
</FlexItem>
<FlexItem isBlock>
<Spacer marginX={ 2 } marginBottom={ 0 }>
<RangeControl
value={ customRangeValue }
min={ 0 }
max={
CUSTOM_VALUE_SETTINGS[ selectedUnit ]?.max ?? 10
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the thinking behind a default max of 10?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, this one was just a copy+paste that I hadn't considered. From looking over some of the other possible unit types that aren't covered here, I think maybe a default max of 100 would be better for this component (for example if a theme allowed ch units). Happy to go with a different value if anyone can think of a better fallback max.

Copy link
Contributor

Choose a reason for hiding this comment

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

A default of 100 was probably more what I was expecting as it's the default for our RangeControl I think.

}
step={
CUSTOM_VALUE_SETTINGS[ selectedUnit ]?.steps ??
0.1
}
withInputField={ false }
onChange={ handleSliderChange }
/>
</Spacer>
</FlexItem>
</Flex>
</fieldset>
);
}
1 change: 1 addition & 0 deletions packages/block-editor/src/components/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ export { default as __experimentalColorGradientControl } from './colors-gradient
export { default as __experimentalColorGradientSettingsDropdown } from './colors-gradients/dropdown';
export { default as __experimentalPanelColorGradientSettings } from './colors-gradients/panel-color-gradient-settings';
export { default as __experimentalUseMultipleOriginColorsAndGradients } from './colors-gradients/use-multiple-origin-colors-and-gradients';
export { default as __experimentalHeightControl } from './height-control';
export {
default as __experimentalImageEditor,
ImageEditingProvider as __experimentalImageEditingProvider,
Expand Down
1 change: 0 additions & 1 deletion packages/block-editor/src/hooks/dimensions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,6 @@ export function DimensionsPanel( props ) {
) }
{ ! isMinHeightDisabled && (
<ToolsPanelItem
className="single-column"
hasValue={ () => hasMinHeightValue( props ) }
label={ __( 'Min. height' ) }
onDeselect={ () => resetMinHeight( props ) }
Expand Down
21 changes: 2 additions & 19 deletions packages/block-editor/src/hooks/min-height.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,13 @@
* WordPress dependencies
*/
import { getBlockSupport } from '@wordpress/blocks';
import {
__experimentalUseCustomUnits as useCustomUnits,
__experimentalUnitControl as UnitControl,
} from '@wordpress/components';
import { __ } from '@wordpress/i18n';

/**
* Internal dependencies
*/
import useSetting from '../components/use-setting';
import HeightControl from '../components/height-control';
import { DIMENSIONS_SUPPORT_KEY } from './dimensions';
import { cleanEmptyObject } from './utils';

Expand Down Expand Up @@ -81,17 +78,6 @@ export function MinHeightEdit( props ) {
setAttributes,
} = props;

const units = useCustomUnits( {
availableUnits: useSetting( 'dimensions.units' ) || [
'%',
'px',
'em',
'rem',
'vh',
'vw',
],
} );

if ( useIsMinHeightDisabled( props ) ) {
return null;
}
Expand All @@ -109,13 +95,10 @@ export function MinHeightEdit( props ) {
};

return (
<UnitControl
<HeightControl
label={ __( 'Min. height' ) }
value={ style?.dimensions?.minHeight }
units={ units }
onChange={ onChange }
min={ 0 }
size={ '__unstable-large' }
/>
);
}
7 changes: 2 additions & 5 deletions packages/edit-site/src/components/global-styles/dimensions-panel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
} from '@wordpress/components';
import {
__experimentalUseCustomSides as useCustomSides,
__experimentalHeightControl as HeightControl,
__experimentalSpacingSizesControl as SpacingSizesControl,
} from '@wordpress/block-editor';
import { Icon, positionCenter, stretchWide } from '@wordpress/icons';
Expand Down Expand Up @@ -556,19 +557,15 @@ export default function DimensionsPanel( { name } ) {
) }
{ showMinHeightControl && (
<ToolsPanelItem
className="single-column"
hasValue={ hasMinHeightValue }
label={ __( 'Min. height' ) }
onDeselect={ resetMinHeightValue }
isShownByDefault={ true }
>
<UnitControl
<HeightControl
label={ __( 'Min. height' ) }
value={ minHeightValue }
onChange={ setMinHeightValue }
units={ units }
min={ 0 }
size={ '__unstable-large' }
/>
</ToolsPanelItem>
) }
Expand Down