-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
updates LayoutTypeSwitcher to use ToggleGroupControl #65498
updates LayoutTypeSwitcher to use ToggleGroupControl #65498
Conversation
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. |
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @spadeshoe! In case you missed it, we'd love to have you join us in our Slack community. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your work here!
<ToggleGroupControlOption | ||
value={ name } | ||
label={ name } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the component APIs are used a bit incorrectly. We'll need the following changes to keep it in sync with the block variation, and for the options to have the correct labels.
diff --git a/packages/block-editor/src/hooks/layout.js b/packages/block-editor/src/hooks/layout.js
index f437a82ac1..90859e5e43 100644
--- a/packages/block-editor/src/hooks/layout.js
+++ b/packages/block-editor/src/hooks/layout.js
@@ -313,7 +313,7 @@ export default {
},
};
-function LayoutTypeSwitcher( { key, onChange } ) {
+function LayoutTypeSwitcher( { type, onChange } ) {
return (
<ToggleGroupControl
__next40pxDefaultSize
@@ -322,16 +322,16 @@ function LayoutTypeSwitcher( { key, onChange } ) {
__nextHasNoMarginBottom
hideLabelFromVision
isAdaptiveWidth
+ value={ type }
+ onChange={ onChange }
>
{ getLayoutTypes().map( ( { name, label } ) => {
return (
<ToggleGroupControlOption
+ key={ name }
value={ name }
- label={ name }
- onClick={ () => onChange( name ) }
- >
- { label }
- </ToggleGroupControlOption>
+ label={ label }
+ />
);
} ) }
</ToggleGroupControl>
CleanShot.2024-09-26.at.09.03.34.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, they can be complicated 😆
<ToggleGroupControl | ||
__next40pxDefaultSize | ||
isBlock | ||
label={ __( 'Layout Type Select' ) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a simpler label would be more in line with all the other labels, what do you think?
label={ __( 'Layout Type Select' ) } | |
label={ __( 'Layout type' ) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is much cleaner and to the point. I made this change along with those you mentioned above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thank you for the follow-up!
* updates LayoutTypeSwitcher to useToggleGroupControl * updates label and corrects LayoutTypeSwitcher component misconfiguration Co-authored-by: spadeshoe <spdft@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org>
I just cherry-picked this PR to the wp/6.7 branch to get it included in the next release: 2233538 |
* updates LayoutTypeSwitcher to useToggleGroupControl * updates label and corrects LayoutTypeSwitcher component misconfiguration Co-authored-by: spadeshoe <spdft@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org>
What?
PR replaces the ButtonGroup component with ToggleGroupControl in the LayoutTypeSwitcher component
Why?
Part Of -#65339
How?
Updates LayoutTypeSwitcher to use the new ToggleGroupControl in lieu of ButtonGroup component
Testing Instructions
Note: the LayoutTypeSwitcher function is no longer used in any core blocks. You will need to adjust the conditional on line 267 to make the controls visible on the block.
! inherit && allowSwitching &&
totrue &&
Testing Instructions for Keyboard
Same as above
Screenshots or screencast