ImageSizeControls: Replace ButtonGroup with ToggleGroupControl #65386

hbhalodia · 2024-09-17T05:33:51Z

What?

PR replaces the ButtonGroup component with ToggleGroupControl in the ImageSizeControls.

Why?

Part Of - Replace ButtonGroup usage with ToggleGroupControl #65339

Testing Instructions

Open any post or page.
Add latest post block.
Enable option to display featured image.
Check for the image size controls.
Should work as expected as before.

Testing Instructions for Keyboard

Same

Screenshots or screencast

Before	After

…l and ToggleGroupControlOption

github-actions · 2024-09-17T05:34:04Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: hbhalodia <[email protected]>
Co-authored-by: ciampo <[email protected]>
Co-authored-by: t-hamano <[email protected]>
Co-authored-by: mirka <[email protected]>
Co-authored-by: tyxla <[email protected]>
Co-authored-by: kevin940726 <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

ciampo · 2024-09-18T21:38:39Z

packages/block-editor/src/components/image-size-control/index.js

+						}
+						value={ addSelectedScaleValue() }
+						isBlock
+						isDeselectable


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 ?

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".

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.

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,

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.

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,

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.

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.

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.

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

packages/block-editor/src/components/image-size-control/index.js

tyxla · 2024-10-17T07:22:20Z

packages/block-editor/src/components/image-size-control/index.js

+								<ToggleGroupControlOption
+									key={ scale }
+									value={ scale }
+									label={ `${ scale }%` }


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

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

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,

I filed issue #66298 regarding localizing percentage values.

…t scaledWidth and scaledHeight

hbhalodia · 2024-10-21T09:14:30Z

Hi @mirka @t-hamano @ciampo @tyxla, I have implemented the suggestion for the utility function implementation. Let me know if I need to address anything with the scope of this issue and PR.

Thank You 🙇.

ciampo · 2024-10-21T10:05:10Z

Looks like unit tests need updating — the ToggleGroupControl component implements radio semantics, and therefore each size option is not associated with the button role. I suggest using:

use screen.getByRole( 'radio', { name: accessibleName } ) to find the individual options
screen.getByRole( 'radio', { name: accessibleName } ).toBeChecked() or .not.toBeChecked() to explicitly assert which option is selected
or use screen.getByRole( 'radio', { name: accessibleName, checked: true } ) to implicitly add the "selected" check when querying for the option

hbhalodia · 2024-10-21T11:47:52Z

Hi @ciampo, I would update the tests, Also to confirm, since we are not adding the reset button, should we remove the related test suite for the same?

Thank You,

ciampo · 2024-10-21T12:12:51Z

Hi @ciampo, I would update the tests, Also to confirm, since we are not adding the reset button, should we remove the related test suite for the same?

Thank You,

I think so. We should make sure that we test the new logic highlighted by Aki in this comment

…omponent

ciampo · 2024-10-24T10:24:06Z

You may want to update this branch with the latest trunk commits in order to solve the CI error (the fix was merged in #66316)

kevin940726 · 2024-10-25T02:23:20Z

As we've already entered the RC phase of 6.7, should we backport this PR to the 6.7 branch? Is this a fix of a regression introduced in the 6.7 cycle?

t-hamano · 2024-10-25T02:30:07Z

Is this a fix of a regression introduced in the 6.7 cycle?

This PR is part of #65339 and looks like an enhancement to me, not a bug.

The following similar PR has already been backported, but this one is a bit more complicated than those two, so it might be safer not to backport it.

mirka · 2024-10-25T02:55:22Z

My bad, this one doesn't contain an accessibility bug like #65340 and #65346 do.

ciampo

All tests are passing, although I left a couple of minor suggestions.

@t-hamano / @mirka , could you take a final look to make sure it matched the behavior you settled on?

packages/block-editor/src/components/image-size-control/test/index.js

ciampo

Approving since all feedback is addressed. I suggest waiting for @t-hamano 's review as well, to test the new overall behavior as he highlighted in a previous conversation.

And thank you for the great work and patience, @hbhalodia 🙏

t-hamano

Thanks for the update! I left one final small feedback, but I think it's ready to ship.

t-hamano · 2024-10-29T03:07:04Z

packages/block-editor/src/components/image-size-control/index.js

+ *
+ * @return {Object} The scaled width and height.
+ */
+function getScaledWidthAndHeight( scale, imageWidth, imageHeight ) {


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.

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

Thank You,

ciampo · 2024-10-29T09:11:06Z

I guess we're good to merge?

…ress#65386) Co-authored-by: hbhalodia <[email protected]> Co-authored-by: ciampo <[email protected]> Co-authored-by: t-hamano <[email protected]> Co-authored-by: mirka <[email protected]> Co-authored-by: tyxla <[email protected]> Co-authored-by: kevin940726 <[email protected]>

Fix replace Button and ButtonGroup component to use ToggleGroupContro…

30dbcb1

…l and ToggleGroupControlOption

hbhalodia requested a review from ellatrix as a code owner September 17, 2024 05:33

hbhalodia mentioned this pull request Sep 17, 2024

Replace ButtonGroup usage with ToggleGroupControl #65339

Closed

4 tasks

mirka requested a review from a team September 18, 2024 14:00

mirka assigned hbhalodia Sep 18, 2024

mirka added [Type] Enhancement A suggestion for improvement. [Package] Block editor /packages/block-editor labels Sep 18, 2024

ciampo changed the title ~~Fix: Replace ButtonGroup usage with ToggleGroupControl - Issue 65339~~ ImageSizeControls: Replace ButtonGroup with ToggleGroupControl Sep 18, 2024

ciampo reviewed Sep 18, 2024

View reviewed changes

Merge branch 'trunk' into fix/issue-65339-task1

05ebec6

tyxla reviewed Sep 25, 2024

View reviewed changes

packages/block-editor/src/components/image-size-control/index.js Outdated Show resolved Hide resolved

packages/block-editor/src/components/image-size-control/index.js Outdated Show resolved Hide resolved

hbhalodia added 2 commits October 14, 2024 09:56

Merge branch 'trunk' into fix/issue-65339-task1

1196a22

Add the logic to set undefined when custom values selected

e740c51

t-hamano reviewed Oct 15, 2024

View reviewed changes

packages/block-editor/src/components/image-size-control/index.js Outdated Show resolved Hide resolved

mirka mentioned this pull request Oct 16, 2024

ToggleGroupControl: Don't set value on focus after a reset #66151

Merged

hbhalodia added 3 commits October 16, 2024 09:55

Merge branch 'trunk' into fix/issue-65339-task1

5ee7630

Updated the logic for adding the selected value to toggle group control

12532be

Merge branch 'trunk' into fix/issue-65339-task1

20d8650

hbhalodia requested review from mirka, t-hamano, ciampo and tyxla October 17, 2024 04:28

tyxla reviewed Oct 17, 2024

View reviewed changes

hbhalodia added 2 commits October 21, 2024 14:27

Merge branch 'trunk' into fix/issue-65339-task1

393bb5d

Feat: Update the imageControl component to use utility function to ge…

c84ff9f

…t scaledWidth and scaledHeight

hbhalodia requested a review from tyxla October 21, 2024 09:13

t-hamano mentioned this pull request Oct 22, 2024

l10n: Make percentage values translatable #66298

Open

2 tasks

Update the failing test for the imageSizeControl after updating the c…

08f19cd

…omponent

AhmarZaidi mentioned this pull request Oct 22, 2024

Update percentage strings to be translatable #66323

Open

mirka added [Type] Enhancement A suggestion for improvement. and removed [Type] Bug An existing feature does not function as intended Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Oct 25, 2024

Merge branch 'trunk' into fix/issue-65339-task1

4979ddd

ciampo reviewed Oct 25, 2024

View reviewed changes

packages/block-editor/src/components/image-size-control/test/index.js Show resolved Hide resolved

packages/block-editor/src/components/image-size-control/test/index.js Show resolved Hide resolved

packages/block-editor/src/components/image-size-control/test/index.js Show resolved Hide resolved

hbhalodia added 2 commits October 28, 2024 11:10

Merge branch 'trunk' into fix/issue-65339-task1

558cd41

update the test suite and combine tests for ImageSizeControls dimensions

4bc666b

hbhalodia requested a review from ciampo October 28, 2024 05:50

ciampo approved these changes Oct 28, 2024

View reviewed changes

t-hamano approved these changes Oct 29, 2024

View reviewed changes

hbhalodia added 2 commits October 29, 2024 09:41

Refactor: move utility function before default export

a3d6c29

Merge branch 'trunk' into fix/issue-65339-task1

4fd8d34

ciampo merged commit f73c3ad into WordPress:trunk Oct 29, 2024
61 checks passed

github-actions bot added this to the Gutenberg 19.6 milestone Oct 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ImageSizeControls: Replace ButtonGroup with ToggleGroupControl #65386

ImageSizeControls: Replace ButtonGroup with ToggleGroupControl #65386

hbhalodia commented Sep 17, 2024

github-actions bot commented Sep 17, 2024 •

edited

Loading

ciampo Sep 18, 2024 •

edited by mirka

Loading

t-hamano Sep 19, 2024

mirka Sep 24, 2024

hbhalodia Sep 25, 2024

hbhalodia Sep 25, 2024

hbhalodia Oct 17, 2024

ciampo Oct 17, 2024

mirka Oct 17, 2024

t-hamano Oct 17, 2024

ciampo Oct 18, 2024

tyxla Oct 17, 2024

ciampo Oct 18, 2024

hbhalodia Oct 21, 2024

t-hamano Oct 22, 2024

hbhalodia commented Oct 21, 2024

ciampo commented Oct 21, 2024 •

edited

Loading

hbhalodia commented Oct 21, 2024

ciampo commented Oct 21, 2024

ciampo commented Oct 24, 2024 •

edited

Loading

kevin940726 commented Oct 25, 2024

t-hamano commented Oct 25, 2024

mirka commented Oct 25, 2024

ciampo left a comment

ciampo left a comment •

edited

Loading

t-hamano left a comment

t-hamano Oct 29, 2024

hbhalodia Oct 29, 2024

ciampo commented Oct 29, 2024

ImageSizeControls: Replace ButtonGroup with ToggleGroupControl #65386

ImageSizeControls: Replace ButtonGroup with ToggleGroupControl #65386

Conversation

hbhalodia commented Sep 17, 2024

What?

Why?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

github-actions bot commented Sep 17, 2024 • edited Loading

ciampo Sep 18, 2024 • edited by mirka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hbhalodia commented Oct 21, 2024

ciampo commented Oct 21, 2024 • edited Loading

hbhalodia commented Oct 21, 2024

ciampo commented Oct 21, 2024

ciampo commented Oct 24, 2024 • edited Loading

kevin940726 commented Oct 25, 2024

t-hamano commented Oct 25, 2024

mirka commented Oct 25, 2024

ciampo left a comment

Choose a reason for hiding this comment

ciampo left a comment • edited Loading

Choose a reason for hiding this comment

t-hamano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ciampo commented Oct 29, 2024

github-actions bot commented Sep 17, 2024 •

edited

Loading

ciampo Sep 18, 2024 •

edited by mirka

Loading

ciampo commented Oct 21, 2024 •

edited

Loading

ciampo commented Oct 24, 2024 •

edited

Loading

ciampo left a comment •

edited

Loading