Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace ButtonGroup usage with ToggleGroupControl #65339

Closed
4 tasks done
Tracked by #65338
mirka opened this issue Sep 13, 2024 · 9 comments
Closed
4 tasks done
Tracked by #65338

Replace ButtonGroup usage with ToggleGroupControl #65339

mirka opened this issue Sep 13, 2024 · 9 comments
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Good First Issue An issue that's suitable for someone looking to contribute for the first time [Type] Code Quality Issues or PRs that relate to code quality

Comments

@mirka
Copy link
Member

mirka commented Sep 13, 2024

Part of #65338

All of our remaining usage of the ButtonGroup component is visually outdated as a segmented control and often inaccessibly implemented. We should correct this by replacing them with ToggleGroupControl.

How to submit PRs

To pick up a task, write your name by the checklist item or post a comment to this issue.

Please include before/after screenshots in the PR.

@mirka mirka changed the title Replace all remaining usage with ToggleGroupControl to correct styling consistency and accessibility issues. Replace ButtonGroup usage with ToggleGroupControl Sep 13, 2024
@mirka mirka added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Code Quality Issues or PRs that relate to code quality Good First Issue An issue that's suitable for someone looking to contribute for the first time labels Sep 13, 2024
@hbhalodia
Copy link
Contributor

Hi @mirka, I want to work on the first issue mentioned in the description.

Thank You,

@hbhalodia
Copy link
Contributor

Hi @mirka - Have raised the PR for the task 1 - #65386

Thank You,

@spadeshoe
Copy link
Contributor

@mirka I would like to work on the packages/block-editor/src/hooks/layout.js part of this issue. I am unsure of what component in the Editor LayoutTypeSwitcher is affecting so I have been unable to test that my change has not broken anything. I thought it might be the type size selector. See screenshot.

Screenshot 2024-09-17 at 2 47 30 PM

@mirka
Copy link
Member Author

mirka commented Sep 18, 2024

@spadeshoe Good question! I don't think this code path is actually reached in any of our core blocks. So you can force the condition like this:

diff --git a/packages/block-editor/src/hooks/layout.js b/packages/block-editor/src/hooks/layout.js
index 22d916d7b7..7a349ceb0b 100644
--- a/packages/block-editor/src/hooks/layout.js
+++ b/packages/block-editor/src/hooks/layout.js
@@ -264,7 +264,7 @@ function LayoutPanelPure( {
 						</>
 					) }
 
-					{ ! inherit && allowSwitching && (
+					{ true && (
 						<LayoutTypeSwitcher
 							type={ blockLayoutType }
 							onChange={ onChangeType }

Then insert a Grid block, for example. And you'll see this:

Layout type switcher

@spadeshoe
Copy link
Contributor

Thanks @mirka The conditional trick worked and I was able to test the update. I created the PR for the change: #65498

@jeryj and I had done some digging Tuesday and discovered what you did, that this code is no longer used in core blocks. I have a branch that removes this code all together. I'm not sure what the protocol is for removing "deprecated" code, if it should be removed at all for backwards compatibility.

@mirka
Copy link
Member Author

mirka commented Sep 24, 2024

@spadeshoe Thanks for the PR, we'll get it reviewed shortly!

I had done some digging Tuesday and discovered what you did, that this code is no longer used in core blocks. I have a branch that removes this code all together. I'm not sure what the protocol is for removing "deprecated" code, if it should be removed at all for backwards compatibility.

Although it isn't used in any of the blocks in core, it's still a supported block API so we'll need to keep it 👍

@t-hamano
Copy link
Contributor

I believe all tasks are complete and would like to close this issue.

@ciampo
Copy link
Contributor

ciampo commented Dec 13, 2024

@mirka , anything left before being able to deprecate ButtonGroup ?

Edit: nevermind, just seen #65429

@mirka
Copy link
Member Author

mirka commented Dec 13, 2024

Last instance that I somehow missed: #67985

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Good First Issue An issue that's suitable for someone looking to contribute for the first time [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

No branches or pull requests

5 participants