Feat: Button groups in Typography tools should use ToggleGroupControl #64529

hbhalodia · 2024-08-15T05:15:08Z

What?

Fixes - Button groups in Typography tools should use ToggleGroupControl #64525

Why?

To make use of components instead of ad hoc buttons.

How?

This PR changes the SegmentedTextControl component, to make use of ToggleGroupControl and ToggleGroupControlOptionIcon to render the same logic as it did with buttons.

Testing Instructions

Open any post/page.
Add any paragraph.
Open typography controls in block.
Check for letter case, orientation, and decoration, it should work as is.

Testing Instructions for Keyboard

NIL

Screenshots or screencast

…nent

github-actions · 2024-08-15T05:15:23Z

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 <hbhalodia@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>

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

t-hamano

Thanks for the PR!

It basically works fine, but there are a few things I think need to be addressed.

The labels are not aligned when placed next to a 40px item:

This is the effect of the padding here:

gutenberg/packages/block-editor/src/components/segmented-text-control/style.scss

Line 8 in cb224d4

padding: $grid-unit-05 0;

We can remove the following custom styles:

gutenberg/packages/block-editor/src/components/segmented-text-control/style.scss

Lines 6 to 14 in cb224d4

    
           .block-editor-segmented-text-control__buttons { 
        
           	// 4px of padding makes the row 40px high, same as an input. 
        
           	padding: $grid-unit-05 0; 
        
           	display: flex; 
        
           } 
        
           .components-button.has-icon { 
        
           	margin-right: $grid-unit-05; 
        
           }

Additionally, if we just remove this style, the horizontal alignment of the 40px sized text field and button will not match:

This problem can be solved by adding __next40pxDefaultSize to the ToggleGroupControl component.

hbhalodia · 2024-08-15T13:18:32Z

Thanks for the PR!

It basically works fine, but there are a few things I think need to be addressed.

The labels are not aligned when placed next to a 40px item:

This is the effect of the padding here:

gutenberg/packages/block-editor/src/components/segmented-text-control/style.scss

Line 8 in cb224d4

padding: $grid-unit-05 0;

We can remove the following custom styles:

gutenberg/packages/block-editor/src/components/segmented-text-control/style.scss

Lines 6 to 14 in cb224d4

.block-editor-segmented-text-control__buttons {

// 4px of padding makes the row 40px high, same as an input.

padding: $grid-unit-05 0;

display: flex;

}

.components-button.has-icon {

margin-right: $grid-unit-05;

}

Additionally, if we just remove this style, the horizontal alignment of the 40px sized text field and button will not match:

This problem can be solved by adding __next40pxDefaultSize to the ToggleGroupControl component.

Thanks @t-hamano, I would update the PR asap with the requested changes.

t-hamano · 2024-08-15T13:19:29Z

While testing this PR, I noticed that when isDeselectable is true in the ToggleGroupControl component, the following behavior occurs:

Outline border doesn't appear when focused or hovered over
Focus is lost when pressing left or right arrow keys

I don't know if this is a bug or expected behavior, but I'll send a ping to the @WordPress/gutenberg-components team.

e34b1b640ceb395431711773de264e8e.mp4

mirka · 2024-08-15T13:24:39Z

While testing this PR, I noticed that when isDeselectable is true in the ToggleGroupControl component, the following behavior occurs:

Thanks for the ping! Yes, this is intended behavior. Enabling isDeselectable changes the underlying HTML from radio semantics to button semantics, so the keyboard navigation (and focus styling) changes accordingly.

Though there have been some discussions to change the visual design more substantially (#64439), so it would cause less confusion around it.

…ith components

hbhalodia · 2024-08-15T13:35:54Z

Hi @t-hamano @mirka, The feedbacks have been addressed. Please take a look on the same.

Thank You,

mirka · 2024-08-15T13:45:35Z

packages/block-editor/src/components/segmented-text-control/index.js

+							/>
+						);
+					} ) }
+				</ToggleGroupControl>
 			</div>
 		</fieldset>


The ToggleGroupControl usage looks good, thank you!

However, the outer fieldset and div are now completely unnecessary, and this SegmentedTextControl component is also unnecessary, as consumers can just use ToggleGroupControl directly. There is nothing that this component adds on top of ToggleGroupControl itself.

Ideally, we would clean that up in this PR, but if you prefer, we can split out the refactoring work to a separate PR. At minimum, I think we should to remove the fieldset and div before landing.

I can update on this PR itself, let me know what suits better. I am open to both.

Thanks

Probably better to have those updates as part of this PR, if you don't mind. Thank you 🙏

The SegmentedTextControl component is a wrapper component introduced in #60841 to make the same code common.

We should be able to simply replace the SegmentedTextControl component with the ToggleGroupControl component.
Also, since the SegmentControl component is not exposed, it is safe to delete it 👍

And thank you @t-hamano for apparently having done the work (#60841) to consolidate all the instances into this unified component 🙏 I had assumed that they were all separate implementations as I had seen before, but pleasantly surprised to see that they were actually consolidated now 😄

Hi Team, apologies, I would be updating these feedbacks by tomorrow. ✨

Hi @mirka @t-hamano @ciampo, Have updated the PR with the requested changes.

Thank You,

mirka · 2024-08-15T14:06:50Z

Focus is lost when pressing left or right arrow keys

@t-hamano Sorry, I misinterpreted this part. Focus loss is indeed strange, and I can't repro on the major three browsers on Mac. Could it be a Windows thing? It's supposed to move through the options with Tab, but arrow keys shouldn't cause focus loss.

Or do you have some kind of special mode enabled? I noticed in your screen recording that the arrow key moved a text cursor into the label 😳 I've never seen that before.

t-hamano · 2024-08-15T14:35:25Z

Or do you have some kind of special mode enabled? I noticed in your screen recording that the arrow key moved a text cursor into the label

After researching this problem, I found that it has to do with the "text cursor mode" of the Chrome browser:

Use a text cursor to navigate and select text - Google Accessibility Help

This means that we can move text with the left and right keys regardless of whether the element is focusable or not. I assumed that this behavior was the default behavior of the Windows OS, but it seems to be a Chrome-specific feature 😅

This feature can be disabled with the F7 key. So what I mentioned may not be a bug.

81445d75b947d5174259f4776daf8ab3.mp4

mirka · 2024-08-15T14:38:10Z

After researching this problem, I found that it has to do with the "text cursor mode" of the Chrome browser

Interesting 😳 Thanks for looking into it!

tyxla

I just gave this a round of testing, and it works/looks great!

Not much to add to the prior feedback, but I just wanted to thank you @hbhalodia for the great work utilizing the right @wordpress/components and cleaning up unnecessary deviation 🚀

ciampo · 2024-08-16T10:46:03Z

I just wanted to thank you @hbhalodia for the great work

Absolutely! Thank you @hbhalodia for your great work. It would be great to continue the collaboration, in case you had more capacity!

Code is looking good on my end, but I'll let @t-hamano and @mirka give the final approval as they've been reviewing the PR from the start.

hbhalodia · 2024-08-16T14:34:51Z

Thank You, @ciampo @tyxla ❤️.

It would be great to continue the collaboration, in case you had more capacity!

Sure, I would love to continue the collaboration 🚀.

mirka

Beautiful, thank you for the cleanup! 🚀

t-hamano · 2024-08-17T10:48:22Z

@hbhalodia Thank you for your great work!

Update the letterCase typogrpahy panel to use toggleGroupOption compo…

d18c729

…nent

hbhalodia requested a review from ellatrix as a code owner August 15, 2024 05:15

hbhalodia mentioned this pull request Aug 15, 2024

Button groups in Typography tools should use ToggleGroupControl #64525

Closed

shail-mehta added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components labels Aug 15, 2024

t-hamano linked an issue Aug 15, 2024 that may be closed by this pull request

Button groups in Typography tools should use ToggleGroupControl #64525

Closed

t-hamano self-requested a review August 15, 2024 13:07

t-hamano reviewed Aug 15, 2024

View reviewed changes

mirka requested a review from a team August 15, 2024 13:24

Remove unecessary style for segmented text control and match height w…

4206c46

…ith components

hbhalodia requested a review from t-hamano August 15, 2024 13:35

mirka reviewed Aug 15, 2024

View reviewed changes

hbhalodia added 2 commits August 16, 2024 09:33

Remove SegmentedTextControl component and use toggleControl

6b0f5a3

Reduce the diff change in the PR

42889a2

hbhalodia requested review from mirka and ciampo August 16, 2024 04:11

tyxla reviewed Aug 16, 2024

View reviewed changes

mirka approved these changes Aug 16, 2024

View reviewed changes

mirka merged commit c459a7b into WordPress:trunk Aug 16, 2024
64 checks passed

mirka removed the [Package] Components /packages/components label Aug 16, 2024

mirka added the [Package] Block editor /packages/block-editor label Aug 16, 2024

github-actions bot added this to the Gutenberg 19.1 milestone Aug 16, 2024

mirka mentioned this pull request Aug 16, 2024

TextDecorationControl, TextTransformControl: Remove size prop in Storybook #64583

Merged

t-hamano mentioned this pull request Sep 23, 2024

Resizable Editor: Make the editor resizable with arrow keys #65546

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: Button groups in Typography tools should use ToggleGroupControl #64529

Feat: Button groups in Typography tools should use ToggleGroupControl #64529

hbhalodia commented Aug 15, 2024

github-actions bot commented Aug 15, 2024 •

edited

Loading

t-hamano left a comment

hbhalodia commented Aug 15, 2024

t-hamano commented Aug 15, 2024

mirka commented Aug 15, 2024

hbhalodia commented Aug 15, 2024

mirka Aug 15, 2024

hbhalodia Aug 15, 2024

ciampo Aug 15, 2024 •

edited

Loading

t-hamano Aug 15, 2024

mirka Aug 15, 2024 •

edited

Loading

hbhalodia Aug 15, 2024

hbhalodia Aug 16, 2024

mirka commented Aug 15, 2024

t-hamano commented Aug 15, 2024

mirka commented Aug 15, 2024

tyxla left a comment

ciampo commented Aug 16, 2024

hbhalodia commented Aug 16, 2024

mirka left a comment

t-hamano commented Aug 17, 2024

	.block-editor-segmented-text-control__buttons {
	// 4px of padding makes the row 40px high, same as an input.
	padding: $grid-unit-05 0;
	display: flex;
	}

	.components-button.has-icon {
	margin-right: $grid-unit-05;
	}

Feat: Button groups in Typography tools should use ToggleGroupControl #64529

Feat: Button groups in Typography tools should use ToggleGroupControl #64529

Conversation

hbhalodia commented Aug 15, 2024

What?

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

github-actions bot commented Aug 15, 2024 • edited Loading

t-hamano left a comment

Choose a reason for hiding this comment

hbhalodia commented Aug 15, 2024

t-hamano commented Aug 15, 2024

mirka commented Aug 15, 2024

hbhalodia commented Aug 15, 2024

mirka Aug 15, 2024

Choose a reason for hiding this comment

hbhalodia Aug 15, 2024

Choose a reason for hiding this comment

ciampo Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

t-hamano Aug 15, 2024

Choose a reason for hiding this comment

mirka Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

hbhalodia Aug 15, 2024

Choose a reason for hiding this comment

hbhalodia Aug 16, 2024

Choose a reason for hiding this comment

mirka commented Aug 15, 2024

t-hamano commented Aug 15, 2024

mirka commented Aug 15, 2024

tyxla left a comment

Choose a reason for hiding this comment

ciampo commented Aug 16, 2024

hbhalodia commented Aug 16, 2024

mirka left a comment

Choose a reason for hiding this comment

t-hamano commented Aug 17, 2024

github-actions bot commented Aug 15, 2024 •

edited

Loading

ciampo Aug 15, 2024 •

edited

Loading

mirka Aug 15, 2024 •

edited

Loading