gutenberg icon indicating copy to clipboard operation
gutenberg copied to clipboard

ImageSizeControls: Replace ButtonGroup with ToggleGroupControl

Open hbhalodia opened this issue 1 year ago • 1 comments

What?

PR replaces the ButtonGroup component with ToggleGroupControl in the ImageSizeControls.

Why?

  • Part Of - #65339

Testing Instructions

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

Testing Instructions for Keyboard

  • Same

Screenshots or screencast

Before After
Screenshot 2024-09-17 at 11 02 49 AM Screenshot 2024-09-17 at 11 01 44 AM

hbhalodia avatar Sep 17 '24 05:09 hbhalodia

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.

github-actions[bot] avatar Sep 17 '24 05:09 github-actions[bot]

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

hbhalodia avatar Oct 21 '24 09:10 hbhalodia

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

ciampo avatar Oct 21 '24 10:10 ciampo

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,

hbhalodia avatar Oct 21 '24 11:10 hbhalodia

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

ciampo avatar Oct 21 '24 12:10 ciampo

You may want to update this branch with the latest trunk commits in order to solve the CI error (the fix was merged in https://github.com/WordPress/gutenberg/pull/66316)

ciampo avatar Oct 24 '24 10:10 ciampo

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?

kevin940726 avatar Oct 25 '24 02:10 kevin940726

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.

  • https://github.com/WordPress/gutenberg/pull/65498
  • https://github.com/WordPress/gutenberg/pull/65340

t-hamano avatar Oct 25 '24 02:10 t-hamano

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

mirka avatar Oct 25 '24 02:10 mirka

I guess we're good to merge?

ciampo avatar Oct 29 '24 09:10 ciampo