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
Changes from 7 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
91 changes: 48 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 @@ -33,6 +33,33 @@ 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 = Math.round( imageWidth * ( scale / 100 ) );
const scaledHeight = Math.round( imageHeight * ( scale / 100 ) );
hbhalodia marked this conversation as resolved.
Show resolved Hide resolved

updateDimensions( scaledHeight, scaledWidth );
};

/**
* Add the stored image preset value to toggle group control.
*/
const selectedValue = IMAGE_SIZE_PRESETS.find( ( scale ) => {
const scaledWidth = Math.round( imageWidth * ( scale / 100 ) );
const scaledHeight = Math.round( imageHeight * ( scale / 100 ) );
return currentWidth === scaledWidth && currentHeight === scaledHeight;
} );

return (
<>
{ imageSizeOptions && imageSizeOptions.length > 0 && (
Expand Down Expand Up @@ -70,47 +97,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
Loading