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.

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

ImageSizeControls: Replace ButtonGroup with ToggleGroupControl #65386

Merged
merged 15 commits into from
Oct 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
117 changes: 74 additions & 43 deletions packages/block-editor/src/components/image-size-control/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@
* WordPress dependencies
*/
import {
Button,
ButtonGroup,
SelectControl,
__experimentalNumberControl as NumberControl,
__experimentalHStack as HStack,
__experimentalToggleGroupControl as ToggleGroupControl,
__experimentalToggleGroupControlOption as ToggleGroupControlOption,
} from '@wordpress/components';
import { __ } from '@wordpress/i18n';

Expand All @@ -18,6 +18,25 @@ import useDimensionHandler from './use-dimension-handler';
const IMAGE_SIZE_PRESETS = [ 25, 50, 75, 100 ];
const noop = () => {};

/**
* Get scaled width and height for the given scale.
*
* @param {number} scale The scale to get the scaled width and height for.
* @param {number} imageWidth The image width.
* @param {number} imageHeight The image height.
*
* @return {Object} The scaled width and height.
*/
function getScaledWidthAndHeight( scale, imageWidth, imageHeight ) {
const scaledWidth = Math.round( imageWidth * ( scale / 100 ) );
const scaledHeight = Math.round( imageHeight * ( scale / 100 ) );

return {
scaledWidth,
scaledHeight,
};
}

export default function ImageSizeControl( {
imageSizeHelp,
imageWidth,
Expand All @@ -33,6 +52,40 @@ export default function ImageSizeControl( {
const { currentHeight, currentWidth, updateDimension, updateDimensions } =
useDimensionHandler( height, width, imageHeight, imageWidth, onChange );

/**
* Updates the dimensions for the given scale.
* Handler for toggle group control change.
*
* @param {number} scale The scale to update the dimensions for.
*/
const handleUpdateDimensions = ( scale ) => {
if ( undefined === scale ) {
updateDimensions();
return;
}

const { scaledWidth, scaledHeight } = getScaledWidthAndHeight(
scale,
imageWidth,
imageHeight
);

updateDimensions( scaledHeight, scaledWidth );
};

/**
* Add the stored image preset value to toggle group control.
*/
const selectedValue = IMAGE_SIZE_PRESETS.find( ( scale ) => {
const { scaledWidth, scaledHeight } = getScaledWidthAndHeight(
scale,
imageWidth,
imageHeight
);

return currentWidth === scaledWidth && currentHeight === scaledHeight;
} );

return (
<>
{ imageSizeOptions && imageSizeOptions.length > 0 && (
Expand Down Expand Up @@ -70,47 +123,25 @@ export default function ImageSizeControl( {
size="__unstable-large"
/>
</HStack>
<HStack>
<ButtonGroup aria-label={ __( 'Image size presets' ) }>
{ IMAGE_SIZE_PRESETS.map( ( scale ) => {
const scaledWidth = Math.round(
imageWidth * ( scale / 100 )
);
const scaledHeight = Math.round(
imageHeight * ( scale / 100 )
);

const isCurrent =
currentWidth === scaledWidth &&
currentHeight === scaledHeight;

return (
<Button
key={ scale }
size="small"
variant={
isCurrent ? 'primary' : undefined
}
isPressed={ isCurrent }
onClick={ () =>
updateDimensions(
scaledHeight,
scaledWidth
)
}
>
{ scale }%
</Button>
);
} ) }
</ButtonGroup>
<Button
size="small"
onClick={ () => updateDimensions() }
>
{ __( 'Reset' ) }
</Button>
</HStack>
<ToggleGroupControl
label={ __( 'Image size presets' ) }
hideLabelFromVision
onChange={ handleUpdateDimensions }
value={ selectedValue }
isBlock
__next40pxDefaultSize
__nextHasNoMarginBottom
>
{ IMAGE_SIZE_PRESETS.map( ( scale ) => {
return (
<ToggleGroupControlOption
key={ scale }
value={ scale }
label={ `${ scale }%` }
Copy link
Member

Choose a reason for hiding this comment

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

It might make sense for the percentage string to be localized. In some languages, it might actually be displayed differently:

https://phrase.com/blog/posts/number-localization/#toc_5

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we localize other percentages across Gutenberg? If not, it may be better to split it to a separate PR to localize all percentages at the same time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not think, we are localizing other percentage implementations, here is the example for column block -

const widthWithUnit = Number.isFinite( width ) ? width + '%' : width;
We are directly using it without localizing.

I agree with @ciampo, to add all at once, so it does not block the scope of this PR.

Thank You,

Copy link
Contributor

Choose a reason for hiding this comment

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

I filed issue #66298 regarding localizing percentage values.

/>
);
} ) }
</ToggleGroupControl>
</div>
) }
</>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -221,47 +221,8 @@ describe( 'ImageSizeControl', () => {
} );
} );

describe( 'reset button', () => {
it( 'resets both height and width to default values', async () => {
const user = userEvent.setup();

render(
<ImageSizeControl
imageHeight="100"
imageWidth="200"
height="300"
width="400"
onChange={ mockOnChange }
/>
);

const heightInput = screen.getByRole( 'spinbutton', {
name: 'Height',
} );
const widthInput = screen.getByRole( 'spinbutton', {
name: 'Width',
} );

// The initial dimension values display first.
expect( heightInput ).toHaveValue( 300 );
expect( widthInput ).toHaveValue( 400 );

await user.click( screen.getByRole( 'button', { name: 'Reset' } ) );

// Both attributes are set to undefined to clear custom values.
expect( mockOnChange ).toHaveBeenLastCalledWith( {
height: undefined,
width: undefined,
} );

// The inputs display the default values once more.
expect( heightInput ).toHaveValue( 100 );
expect( widthInput ).toHaveValue( 200 );
} );
} );

describe( 'image size percentage presets', () => {
it( 'updates height and width attributes on selection', async () => {
it( 'updates height and width on selection', async () => {
const user = userEvent.setup();

render(
Expand All @@ -272,44 +233,25 @@ describe( 'ImageSizeControl', () => {
/>
);

const button = screen.getByRole( 'button', {
const button = screen.getByRole( 'radio', {
name: '50%',
pressed: false,
hbhalodia marked this conversation as resolved.
Show resolved Hide resolved
checked: false,
} );

await user.click( button );

expect( button ).toHaveClass( 'is-pressed' );
ciampo marked this conversation as resolved.
Show resolved Hide resolved
expect( button ).toBeChecked();

// Both attributes are set to the rounded scaled value.
expect( mockOnChange ).toHaveBeenLastCalledWith( {
height: 50,
width: 101,
} );
} );

it( 'updates height and width inputs on selection', async () => {
const user = userEvent.setup();

render(
<ImageSizeControl
imageHeight="100"
imageWidth="201"
onChange={ mockOnChange }
/>
);

const button = screen.getByRole( 'button', {
name: '50%',
pressed: false,
hbhalodia marked this conversation as resolved.
Show resolved Hide resolved
} );

await user.click( button );

// Both attributes are set to the rounded scaled value.
expect(
screen.getByRole( 'spinbutton', { name: 'Height' } )
).toHaveValue( 50 );

expect(
screen.getByRole( 'spinbutton', { name: 'Width' } )
).toHaveValue( 101 );
Expand Down
Loading