-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Button: Migrate to text-align block support #73732
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
base: trunk
Are you sure you want to change the base?
Conversation
|
Size Change: +366 B (+0.01%) Total Size: 2.57 MB
ℹ️ View Unchanged
|
Mamaduka
left a comment
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.
Given the amount of code required to migrate to the block support flag, what's the real benefit here?
Why is the deprecation needed? Because of the missing has-text-align-* class?
| borderProps.className, | ||
| typographyProps.className, | ||
| { | ||
| [ `has-text-align-${ textAlign }` ]: textAlign, |
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 could be considered a breaking change if themes or extenders rely on this class.
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.
The has-text-align-${ textAlign } class is provided by getTypographyClassesAndStyles, so your markup should remain unchanged.
gutenberg/packages/block-editor/src/hooks/use-typography-props.js
Lines 48 to 50 in 2dbaeb1
| const textAlignClassName = !! attributes?.style?.typography?.textAlign | |
| ? `has-text-align-${ attributes?.style?.typography?.textAlign }` | |
| : ''; |
|
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. |
|
Migrate to block support will allow you to set global styles. Once we move to block support, you can set this in global styles. |
| }, | ||
| migrate: migrateTextAlign, | ||
| }; | ||
|
|
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.
A lot of these deprecated versions will become unnecessary after #73116 so I think this is ok for now. and I do think there's a lot of benefits to this unification. (Global Styles support being the important one)
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 we need to add a "migration hook" to the edit function (like we did in the paragraph block) for folks that toggle the old attributes dynamically use updateBlockAttributes action.
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.
youknowriad
left a comment
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 looks good to me
| return; | ||
| } | ||
| lastUpdatedAlignRef.current = textAlign; | ||
| updateStyleWithAlign(); |
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.
You could use a new setter method for setAttributes that removes the need for the useEvent hook and for passing style as an effect dependency.
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! I tried to adjust it, how does this look? 8a32ace
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 what I had in mind, though it's not a blocker; I just wanted to share.
Example patch
diff --git a/packages/block-library/src/utils/deprecated-text-align-attributes.js b/packages/block-library/src/utils/deprecated-text-align-attributes.js
index 0daa1405bd..4dd591f987 100644
--- a/packages/block-library/src/utils/deprecated-text-align-attributes.js
+++ b/packages/block-library/src/utils/deprecated-text-align-attributes.js
@@ -1,7 +1,6 @@
/**
* WordPress dependencies
*/
-import { useEvent } from '@wordpress/compose';
import { useEffect, useRef } from '@wordpress/element';
import deprecated from '@wordpress/deprecated';
import { useDispatch } from '@wordpress/data';
@@ -18,7 +17,12 @@ export default function useDeprecatedTextAlign( props ) {
const { textAlign } = attributes;
const { __unstableMarkNextChangeAsNotPersistent } =
useDispatch( blockEditorStore );
- const updateStyleWithAlign = useEvent( () => {
+ const lastUpdatedAlignRef = useRef();
+ useEffect( () => {
+ if ( textAlign === lastUpdatedAlignRef.current ) {
+ return;
+ }
+ lastUpdatedAlignRef.current = textAlign;
deprecated( `textAlign attribute in ${ name }`, {
alternative: 'style.typography.textAlign',
since: '7.0',
@@ -33,13 +37,10 @@ export default function useDeprecatedTextAlign( props ) {
},
},
} ) );
- } );
- const lastUpdatedAlignRef = useRef();
- useEffect( () => {
- if ( textAlign === lastUpdatedAlignRef.current ) {
- return;
- }
- lastUpdatedAlignRef.current = textAlign;
- updateStyleWithAlign();
- }, [ textAlign, updateStyleWithAlign ] );
+ }, [
+ __unstableMarkNextChangeAsNotPersistent,
+ name,
+ setAttributes,
+ textAlign,
+ ] );
}
What?
Part of #60763
Related to #73111
Migrates the Button block to use the textAlign block support instead of a custom
textAlignattribute. As a consequence it also enables global styles support fortextAlignfor the Button block.Why?
The Button block currently implements its own text alignment logic with a custom align attribute, duplicating code that should be handled by the centralized Button block support. This migration reduces code duplication and consolidates alignment handling across blocks.
How?
Replaces the custom logic with the block supports, adds deprecation and fixes transforms.
Just like
migrateFontFamily, I created themigrateTextAlignfunction so that it can be used in other blocks.Testing Instructions