Block editor: hooks: avoid getEditWrapperProps #56912
Merged
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
|
Size Change: -90 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in bf0950d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7166099199
|
ellatrix
commented
Dec 8, 2023
| // BlockListBlock instead of using getEditWrapperProps then the value is | ||
| // clobbered when the core/style/addEditProps filter runs. | ||
|
|
||
| // TODO: We can do the thing above now. |
There was a problem hiding this comment.
I'm going to avoid refactoring this further, but we can use hooks now.
|
Just testing with a bunch of styles and so far I've noticed that this PR doesn't add
Example HTML<!-- wp:group {"style":{"border":{"width":"54px","color":"#b5bdbc","radius":"34px"},"elements":{"link":{"color":{"text":"var:preset|color|accent-3"}}},"typography":{"fontStyle":"normal","fontWeight":"500","textDecoration":"underline","textTransform":"uppercase"},"spacing":{"blockGap":"var:preset|spacing|50"},"dimensions":{"minHeight":"42px"},"background":{"backgroundImage":{"url":"http://localhost:8888/wp-content/uploads/2023/11/white-bellied-swallow.jpg","id":9,"source":"file","title":"white-bellied-swallow"}}},"textColor":"accent-3","layout":{"type":"constrained"},"fontSize":"large"} -->
<div class="wp-block-group has-border-color has-accent-3-color has-text-color has-link-color has-large-font-size" style="border-color:#b5bdbc;border-width:54px;border-radius:34px;min-height:42px;font-style:normal;font-weight:500;text-decoration:underline;text-transform:uppercase"><!-- wp:paragraph {"dropCap":true,"style":{"typography":{"textDecoration":"line-through","letterSpacing":"23px"}}} -->
<p class="has-drop-cap" style="letter-spacing:23px;text-decoration:line-through">Block with lots of styles</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph {"dropCap":true,"style":{"typography":{"textDecoration":"line-through","letterSpacing":"23px"}}} -->
<p class="has-drop-cap" style="letter-spacing:23px;text-decoration:line-through">Block with lots of styles</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph {"dropCap":true,"style":{"typography":{"textDecoration":"line-through","letterSpacing":"23px"}}} -->
<p class="has-drop-cap" style="letter-spacing:23px;text-decoration:line-through">Block with lots of styles</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group --> |
ramonjd
reviewed
Dec 11, 2023
| attributes, | ||
| skipPaths = skipSerializationPathsSave | ||
| ) { | ||
| if ( ! hasStyleSupport( blockType ) ) { | ||
| if ( ! hasStyleSupport( blockNameOrType ) ) { | ||
| return props; | ||
| } | ||
|
|
||
| let { style } = attributes; | ||
| Object.entries( skipPaths ).forEach( ( [ indicator, path ] ) => { | ||
| const skipSerialization = | ||
| skipSerializationPathsSaveChecks[ indicator ] || |
There was a problem hiding this comment.
Quick question for @andrewserong: how should the background image styles be applied? They seem to be skipped here.
Or should we be also using addSaveProps in hooks/background.js? Trying to work out why the CSS isn't being applied to the group container 🤔
There was a problem hiding this comment.
Good catch! Hrm, at first glance, it looks like this PR likely breaks it. The idea was for addEditProps to be adding the inline style in the edit context, but not serialize it on save.
We'll likely soon have other background image properties that need to be output in the editor view but not serialized to post content.
Or should we be also using addSaveProps in hooks/background.js?
I think that should work! I think we'd just call addSaveProps in hooks/background.js with a skipPaths param that allows the background image styles to be output in that context. What do you think?
There was a problem hiding this comment.
I'll have a look at it, thanks for testing. Would be great if at some point we add e2e tests.
There was a problem hiding this comment.
I found the issue: I used skipSerializationPathsSave instead of skipSerializationPathsEdit. Fixed it in bf0950d.
ellatrix
force-pushed
the
try/hooks-edit-wrapper-props
branch
from
2b286ff to
bf0950d
Compare
ramonjd
approved these changes
Dec 11, 2023
andrewserong
approved these changes
Dec 11, 2023
There was a problem hiding this comment.
This is testing great for me!
✅ Border, color, font-family, font-size, and other style output appears to work as on trunk
✅ Classnames are output as expected
✅ Background image works as on trunk, without the value being serialized to post content 👍
This also fixes up a bug with className output that I encountered separately.
LGTM! ✨
Comment on lines
+57
to
+61
| // May be set to undefined, so check if the property is set! | ||
| if ( | ||
| propsA?.hasOwnProperty( 'className' ) && | ||
| propsB?.hasOwnProperty( 'className' ) | ||
| ) { |
There was a problem hiding this comment.
Fantastic! I just ran into an issue with applying a className for another block support I'm working on and was confused as to why the className wasn't being output consistently, and this fixes it 🙂
There was a problem hiding this comment.
That’s great! Was hoping this makes it also easier to maintain!
Comment on lines
+201
to
+204
| fontSize: getTypographyFontSizeValue( | ||
| { size: fontSize }, | ||
| fluidTypographySettings | ||
| ), |
There was a problem hiding this comment.
I'm not too sure what the right fix is, but I think this change in this PR means that the fluid typography rules are no longer calculated and output when a user enters a custom font-size within the block editor. For example, in the following screenshot, the font-size is the direct value of 121px, but it should be getting the fluid typography rules:
Because this hook returns early if fontSize (as an attribute) isn't present, we never get to this transformation of the value when someone enters a value in style.typography.fontSize.
Found while testing #57866 (review)
There was a problem hiding this comment.
Thanks for spotting this @andrewserong and apologies that I didn't spot it here first.
I'm not sure what the best approach is either.
I hacked something that seems to work in principle, but the output is somehow modified before it reaches the frontend.
The fluid fontSize is calculated correctly, e.g., clamp(46.987px, 2.937rem + ((1vw - 3.2px) * 4.689), 92px), but in the browser inspector (Chrome and FF) I'm seeing something completely different: clamp(46.987px, 2.937rem + 4.689vw - 15.0048px, 92px); 🤔
Visually they look the same. I ran out of time to debug.
Diff of my hack
diff --git a/packages/block-editor/src/components/global-styles/typography-utils.js b/packages/block-editor/src/components/global-styles/typography-utils.js
index cd1340033e..a73dfc3725 100644
--- a/packages/block-editor/src/components/global-styles/typography-utils.js
+++ b/packages/block-editor/src/components/global-styles/typography-utils.js
@@ -81,7 +81,7 @@ export function getTypographyFontSizeValue( preset, typographyOptions ) {
return defaultSize;
}
-function isFluidTypographyEnabled( typographySettings ) {
+export function isFluidTypographyEnabled( typographySettings ) {
const fluidSettings = typographySettings?.fluid;
return (
true === fluidSettings ||
diff --git a/packages/block-editor/src/hooks/font-size.js b/packages/block-editor/src/hooks/font-size.js
index 89491da44e..b643c2a087 100644
--- a/packages/block-editor/src/hooks/font-size.js
+++ b/packages/block-editor/src/hooks/font-size.js
@@ -26,6 +26,7 @@ import { store as blockEditorStore } from '../store';
import {
getTypographyFontSizeValue,
getFluidTypographyOptionsFromSettings,
+ isFluidTypographyEnabled,
} from '../components/global-styles/typography-utils';
export const FONT_SIZE_SUPPORT_KEY = 'typography.fontSize';
@@ -155,33 +156,6 @@ export function useIsFontSizeDisabled( { name: blockName } = {} ) {
function useBlockProps( { name, fontSize, style } ) {
const [ fontSizes ] = useSettings( 'typography.fontSizes' );
-
- // Only add inline styles if the block supports font sizes,
- // doesn't skip serialization of font sizes,
- // doesn't already have an inline font size,
- // and does have a class to extract the font size from.
- if (
- ! hasBlockSupport( name, FONT_SIZE_SUPPORT_KEY ) ||
- shouldSkipSerialization( name, TYPOGRAPHY_SUPPORT_KEY, 'fontSize' ) ||
- ! fontSize
- ) {
- return;
- }
-
- let props = {};
-
- if ( ! style?.typography?.fontSize ) {
- props = {
- style: {
- fontSize: getFontSize(
- fontSizes,
- fontSize,
- style?.typography?.fontSize
- ).size,
- },
- };
- }
-
// TODO: This sucks! We should be using useSetting( 'typography.fluid' )
// or even useSelect( blockEditorStore ). We can't do either here
// because getEditWrapperProps is a plain JavaScript function called by
@@ -194,18 +168,35 @@ function useBlockProps( { name, fontSize, style } ) {
const fluidTypographySettings = getFluidTypographyOptionsFromSettings(
select( blockEditorStore ).getSettings().__experimentalFeatures
);
-
- if ( fontSize ) {
- props = {
- style: {
- fontSize: getTypographyFontSizeValue(
- { size: fontSize },
- fluidTypographySettings
- ),
- },
- };
+ // Only add inline styles if the block supports font sizes,
+ // doesn't skip serialization of font sizes,
+ // doesn't already have an inline font size,
+ // and does have a class to extract the font size from.
+ if (
+ ! hasBlockSupport( name, FONT_SIZE_SUPPORT_KEY ) ||
+ shouldSkipSerialization( name, TYPOGRAPHY_SUPPORT_KEY, 'fontSize' ) ||
+ ( ! fontSize &&
+ ! isFluidTypographyEnabled( fluidTypographySettings ) &&
+ !! style?.typography?.fontSize )
+ ) {
+ return;
}
+ const props = {
+ style: {
+ fontSize: ! style?.typography?.fontSize
+ ? getFontSize(
+ fontSizes,
+ fontSize,
+ style?.typography?.fontSize
+ ).size
+ : getTypographyFontSizeValue(
+ { size: style.typography.fontSize },
+ fluidTypographySettings
+ ),
+ },
+ };
+
return addSaveProps( props, name, { fontSize } );
}
There was a problem hiding this comment.
Should we prioritize this regression for 6.5?
@ellatrix @andrewserong I'll throw a PR up today - would be good to get your 👀 on it to make sure I haven't broken anything.
👍🏻
There was a problem hiding this comment.
Should we prioritize this regression for 6.5?
Yes, that'd be great if you time to. Thanks for picking this up!
This was referenced Jan 30, 2024
2 tasks
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?
We shouldn't extend block settings and add
getEditWrapperPropsfunctions. We should useuseBlockPropsinstead, which makes the code more readable and easier to understand.Why?
How?
Testing Instructions
It's a refactoring: everything should work as before and tests should pass.
Testing Instructions for Keyboard
Screenshots or screencast