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

ImageSizeControls: Replace ButtonGroup with ToggleGroupControl #65386

Merged
merged 15 commits into from
Oct 29, 2024
Merged

ImageSizeControls: Replace ButtonGroup with ToggleGroupControl #65386

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
30dbcb1
Fix replace Button and ButtonGroup component to use ToggleGroupContro…
hbhalodia Sep 17, 2024
05ebec6
Merge branch 'trunk' into fix/issue-65339-task1
hbhalodia Sep 25, 2024
1196a22
Merge branch 'trunk' into fix/issue-65339-task1
hbhalodia Oct 14, 2024
e740c51
Add the logic to set undefined when custom values selected
hbhalodia Oct 14, 2024
5ee7630
Merge branch 'trunk' into fix/issue-65339-task1
hbhalodia Oct 16, 2024
12532be
Updated the logic for adding the selected value to toggle group control
hbhalodia Oct 16, 2024
20d8650
Merge branch 'trunk' into fix/issue-65339-task1
hbhalodia Oct 17, 2024
393bb5d
Merge branch 'trunk' into fix/issue-65339-task1
hbhalodia Oct 21, 2024
c84ff9f
Feat: Update the imageControl component to use utility function to ge…
hbhalodia Oct 21, 2024
08f19cd
Update the failing test for the imageSizeControl after updating the c…
hbhalodia Oct 22, 2024
4979ddd
Merge branch 'trunk' into fix/issue-65339-task1
hbhalodia Oct 25, 2024
558cd41
Merge branch 'trunk' into fix/issue-65339-task1
hbhalodia Oct 28, 2024
4bc666b
update the test suite and combine tests for ImageSizeControls dimensions
hbhalodia Oct 28, 2024
a3d6c29
Refactor: move utility function before default export
hbhalodia Oct 29, 2024
4fd8d34
Merge branch 'trunk' into fix/issue-65339-task1
hbhalodia Oct 29, 2024
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
View file
Open in desktop
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,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,49 +104,46 @@ 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 -

gutenberg/packages/block-library/src/column/edit.js

Line 84 in 9b0f6aa

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>
) }
</>
);
}

/**
* 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 ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: While there are no hard and fast rules, it is generally a good idea to put any sub-components, hooks, or functions used internally by a component before the default exported components.

So, it might be a good idea to move this function 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.

Hi @t-hamano, Updated the PR to move the function before default export.

Thank You,

const scaledWidth = Math.round( imageWidth * ( scale / 100 ) );
const scaledHeight = Math.round( imageHeight * ( scale / 100 ) );

return {
scaledWidth,
scaledHeight,
};
}
68 changes: 5 additions & 63 deletions packages/block-editor/src/components/image-size-control/test/index.js
View file
Open in desktop
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