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

Changes from 1 commit
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
108 changes: 65 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,47 @@ 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 ) => {
/**
* Check if scale is deselected from toggle group control option.
*/
if ( undefined === scale ) {
/**
* Update dimensions to default.
*/
updateDimensions();
return;
}

/**
* Calculate scaled width and height.
*/
const scaledWidth = Math.round( imageWidth * ( scale / 100 ) );
const scaledHeight = Math.round( imageHeight * ( scale / 100 ) );
hbhalodia marked this conversation as resolved.
Show resolved Hide resolved

/**
* Update dimensions.
*/
hbhalodia marked this conversation as resolved.
Show resolved Hide resolved
updateDimensions( scaledHeight, scaledWidth );
};

/**
* Handler for adding the value to toggle group control.
*/
const addSelectedScaleValue = () => {
/**
* Calculate scale size based on current width and image width.
*/
const scaleSize = Math.round( currentWidth * ( 100 / imageWidth ) );
return scaleSize;
};

return (
<>
{ imageSizeOptions && imageSizeOptions.length > 0 && (
Expand Down Expand Up @@ -70,47 +111,28 @@ 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={ ( scale ) =>
handleUpdateDimensions( scale )
}
hbhalodia marked this conversation as resolved.
Show resolved Hide resolved
value={ addSelectedScaleValue() }
isBlock
isDeselectable
Copy link
Contributor

@ciampo ciampo Sep 18, 2024 •
edited by mirka
Loading

Choose a reason for hiding this comment

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

As discussed here, isDeselectable doesn't really work with text options — we should remove it.

One option could be to add an "Auto" option which replaces the 'reset' button. What do you think, @mirka @t-hamano ?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we remove the isDeselectable prop, keyboard users won't be able to set a custom value:

71dcd41756d30d74f869d014456c5284.mp4

As I commented here too, we might need to add an option called "Custom".

Copy link
Member

Choose a reason for hiding this comment

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

keyboard users won't be able to set a custom value

This isn't inherently true — it's only because the logic of addSelectedScaleValue doesn't account for it. In fact the current logic sets invalid values on the ToggleGroupControl, when it should be set back to undefined instead. (cc @hbhalodia)

For the sake of this PR, I'm ok with just replicating the existing functionality. And as it turns out, the Reset button doesn't actually reset, it just sets it to 100%, which is different from an actual reset. So to actually reset, the user needs to clear the width/height input fields. I think that means we don't even need a Reset button nor a "Custom" option to replicate the existing functionality.

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 @mirka, If I had read it correct, need to update the logic on addSelectedScaleValue() function should return undefined, when we set custom values? is that something need to changed.

Thank You,

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's only because the logic of addSelectedScaleValue doesn't account for it. In fact the current logic sets invalid values on the ToggleGroupControl, when it should be set back to undefined instead.

Hi @mirka, I tried to set directly undefined on that logic, seems like anyhow keyboard user would ultimately land on the first option of the ToggleGroupControl, which ultimately sets the first option even we have undefined set on that logic.

Can you please let me know how to set it undefined for keyboard users on that logic, because ultimately when user lands on the toggleGroupControl it would call handleUpdateDimension() which ultimately sets the dimension based on the scale size, and it would remove the custom dimension set.

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 @mirka @t-hamano, This is now looking good. Below is the demo screencast. I guess we can go ahead with the PR, let me know if I need to address any other feedbacks.

GBissue-65339.mov

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 may have lost the plot a little bit, but I noticed that unit tests are failing because they require a "reset" button. With the changes from this PR, would a user be able to reset the value at all? Is that something that we should retain?

Again, apologies in case I missed the part of the conversation where we reached an agreement on that aspect.

Copy link
Member

Choose a reason for hiding this comment

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

would a user be able to reset the value at all? Is that something that we should retain?

The reset button in the original was the functional equivalent of re-selecting the "100%" option. The user can also still delete the values from the number inputs. So, I'd say no functionality is lost.

Copy link
Contributor

Choose a reason for hiding this comment

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

With the changes from this PR, would a user be able to reset the value at all? Is that something that we should retain?

In the current specification, when we press the reset button, the width/height values ​​are set and 100% is selected. These values ​​are not saved in the database. In addition, when we reload the browser, the width/height values ​​and the 100% selection are cleared.

This lack of consistency bothers me, but at least the reset action is considered to be the same as 100%, so I think a reset button is not necessary.

4ba662ea53e68b1a6cfa11351a23a5db.mp4

The presence or absence of a reset button has been discussed in other PRs, and there may not be a complete consensus yet, but my understanding is as follows:

  • Search Block (#65340): If we want to reset the value, we need to empty the custom value input field.
  • Button Block (#65346): Since there is no custom input field, there needs to be some way to make the value empty (auto). One approach being considered is adding an "Auto" option.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you, Lena and Aki — much more clear now. We established that no functionality is lost.

The lack of consistency is also bothering me. I say we merge this PR with the current UX, and then we look at opportunities for improving consistency across other PRs

__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>
) }
</>
Expand Down
Loading