ImageSizeControls: Replace ButtonGroup with ToggleGroupControl
What?
PR replaces the ButtonGroup component with ToggleGroupControl in the ImageSizeControls.
Why?
- Part Of - #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 |
|---|---|
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.
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 🙇.
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
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,
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
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)
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?
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
My bad, this one doesn't contain an accessibility bug like #65340 and #65346 do.
I guess we're good to merge?