Search: Replace ButtonGroup usage with ToggleGroupControl #65340
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Mamaduka
added
[Focus] Accessibility (a11y)
Mamaduka
commented
Sep 13, 2024
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
4 tasks
|
Size Change: +17 B (0%) Total Size: 1.81 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 4f5b3d7. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10855707242
|
ciampo
reviewed
Sep 16, 2024
|
I plan on looking at #65340 (comment) soon, I'm just going through a lot pings these days. Sorry for the delay! |
|
I think this is a bit of a tricky issue because, unlike #65346, the Search block allows us to input a custom value via a If 08d781280034166d094ad2cd6a111eb4.mp4In my opinion, one approach to solving this is to add a "Custom" option. This is incomplete, but here's the diff: Detailsdiff --git a/packages/block-library/src/search/edit.js b/packages/block-library/src/search/edit.js
index 7e7cd855a9..6209469b20 100644
--- a/packages/block-library/src/search/edit.js
+++ b/packages/block-library/src/search/edit.js
@@ -371,6 +371,10 @@ export default function SearchEdit( {
);
};
+ const isOptionsValue = PERCENTAGE_WIDTHS.some(
+ ( widthValue ) => widthValue === width && '%' === widthUnit
+ );
+
const controls = (
<>
<BlockControls>
@@ -449,23 +453,25 @@ export default function SearchEdit( {
/>
<ToggleGroupControl
label={ __( 'Percentage Width' ) }
- value={
- PERCENTAGE_WIDTHS.includes( width ) &&
- widthUnit === '%'
- ? width
- : undefined
- }
+ value={ isOptionsValue ? width : 'custom' }
hideLabelFromVision
onChange={ ( newWidth ) => {
- setAttributes( {
- width: newWidth,
- widthUnit: '%',
- } );
+ if ( newWidth !== 'custom' ) {
+ setAttributes( {
+ width: newWidth,
+ widthUnit: '%',
+ } );
+ }
} }
isBlock
__next40pxDefaultSize
__nextHasNoMarginBottom
>
+ <ToggleGroupControlOption
+ key="custom"
+ value="custom"
+ label={ __( 'Custom' ) }
+ />
{ PERCENTAGE_WIDTHS.map( ( widthValue ) => {
return (
<ToggleGroupControlOptionThis should allow the user to move focus via the keyboard while keeping the custom value: 0da110e15eba28814fad35445cd2dbe8.mp4However, because the |
|
Mhh, it looks like the fact that I guess whether we should wait until the design discussed in #64439 (comment) (and following comments) is implemented, which should allow the component to receive focus without automatically selecting an item. What do folks think? |
2 tasks
mirka
added
[Type] Bug
I would prefer not waiting, since this is already an accessibility issue, and we'll need some kind of reset mechanism anyway. |
Mamaduka
force-pushed
the
update/search-block-button-group
branch
from
b6ba611 to
c1830c9
Compare
|
@mirka, is this PR good to land in its current shape? |
With the changes in this PR, a user would automatically select a value every time keyboard focus goes on |
|
If we ship this PR as is, I think it will cause problems for keyboard users. For example, suppose a user wants to set an additional CSS class after setting a custom width on the Search block. To keep the custom widths, users need to use a hack to close the Settings panel once or update the additional CSS and then update the custom widths again. da318c0bc83fff9deadff28fc388f127.mp4In addition, I tried adding a Custom option. Unfortunately, it seems that it may not be displayed properly depending on the language. Below are screenshots for Japanese and Italian locales. In the latest status of this PR, I noticed that Japanese:
Italiano
The only way I can think of to solve these problems is to disable auto-selection for the |
Ugh. That's my fault. Forgot to pull changes I pushed from a different device 🤦♂️ Agreed. The focus bug is a blocker. Let's try to resolve/fix #62981 and then continue work here. |
|
@Mamaduka The focus bug has been resolved 🎉 |
2 tasks
Mamaduka
force-pushed
the
update/search-block-button-group
branch
from
c1830c9 to
475a7e1
Compare
|
Rebased and updated based on feedback. |
Mamaduka
force-pushed
the
update/search-block-button-group
branch
from
475a7e1 to
3516161
Compare
|
I tested it again, and it doesn't seem to work properly if a custom value is entered: 0e82dd73169ef290bcc5a9fbd2b0856e.mp4This is probably because the We may need to create a value for the const selectedValue = PERCENTAGE_WIDTHS.find( ( presetWidth ) => {
return widthUnit === '%' && width === presetWidth;
} ); |
|
This PR is working as expected. Should we fix the issue with 3a10c080b53dce5e89500709ac880b8d.mp4 |
Which issue are we talking about here? |
I'm referring to the strange behavior mentioned in this comment. Does this mean that there is something that needs to be fixed in the ToggleGroupControl component itself? |
|
If I understood @Mamaduka 's comment above, the issue was in the search block and not in the |
|
Thanks everyone for the feedback ❤️ |
mirka
added
the
Backport to WP 6.7 Beta/RC
gutenbergplugin
pushed a commit
that referenced
this pull request
Oct 24, 2024
Co-authored-by: Mamaduka <[email protected]> Co-authored-by: ciampo <[email protected]> Co-authored-by: tyxla <[email protected]> Co-authored-by: t-hamano <[email protected]> Co-authored-by: mirka <[email protected]>
github-actions
bot
added
Backported to WP Core
|
I just cherry-picked this PR to the wp/6.7 branch to get it included in the next release: 154ceb5 |
karthick-murugan
pushed a commit
to karthick-murugan/gutenberg
that referenced
this pull request
Nov 13, 2024
…65340) Co-authored-by: Mamaduka <[email protected]> Co-authored-by: ciampo <[email protected]> Co-authored-by: tyxla <[email protected]> Co-authored-by: t-hamano <[email protected]> Co-authored-by: mirka <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
PR replaces the
ButtonGroupcomponent withToggleGroupControlin the Search block controls.Why?
Part of #65339.
Testing Instructions
Screenshots or screencast