-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Media & Text: Switch default alignment to none
#48404
Media & Text: Switch default alignment to none
#48404
Conversation
none
none
Size Change: +231 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
Flaky tests detected in e7d1fb7. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4423248120
|
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 have followed the testing instructions, and the PR works well.
I added blocks with each alignment and a placeholder (no media selected); they all show correctly when the plugin is re-activted.
New blocks use the content width, none.
Thank you for adding the additional context to deprecated.js.
Thanks, for giving this a run @carolinan 👍 Given @talldan raised that this has been attempted for other blocks in the past and there were some hurdles in doing so, I'll seek some extra approvals here to give us the best chance to surface any unforeseen problems before we merge. |
none
none
04b0844
to
b099540
Compare
Rebased to see if that fixes the failing mobile e2e test as it looks unrelated. |
It works well in testing, I wasn't able to spot any issues. TBH, I have a hard time understanding how it works! I've always found |
Thanks for testing 🙇
I feel the same way every time I have to touch deprecations. I'll go through this PR tomorrow and give a self-review explaining what I can. |
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've updated the PR description to hopefully better explain the deprecation in this PR. There are also now some inline comments.
Let me know if I've got any of the logic wrong.
const v6ImageFillStyles = ( url, focalPoint ) => { | ||
return url | ||
? { | ||
backgroundImage: `url(${ url })`, | ||
backgroundPosition: focalPoint | ||
? `${ Math.round( focalPoint.x * 100 ) }% ${ Math.round( | ||
focalPoint.y * 100 | ||
) }%` | ||
: `50% 50%`, | ||
} | ||
: {}; | ||
}; | ||
|
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 imageFillStyles
function was changed for the previous version, we need to use that same calculation here to keep the styles as they were for the v6 saved markup.
const migrateDefaultAlign = ( attributes ) => { | ||
if ( attributes.align ) { | ||
return attributes; | ||
} | ||
|
||
return { | ||
...attributes, | ||
align: 'wide', | ||
}; | ||
}; | ||
|
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 separate helper is used here so it can be composed with other attribute migrations for older deprecations. It will only update the align
attribute if not already set.
const v6Attributes = { | ||
...v4ToV5BlockAttributes, | ||
mediaAlt: { | ||
type: 'string', | ||
source: 'attribute', | ||
selector: 'figure img', | ||
attribute: 'alt', | ||
default: '', | ||
__experimentalRole: 'content', | ||
}, | ||
mediaId: { | ||
type: 'number', | ||
__experimentalRole: 'content', | ||
}, | ||
mediaUrl: { | ||
type: 'string', | ||
source: 'attribute', | ||
selector: 'figure video,figure img', | ||
attribute: 'src', | ||
__experimentalRole: 'content', | ||
}, | ||
href: { | ||
type: 'string', | ||
source: 'attribute', | ||
selector: 'figure a', | ||
attribute: 'href', | ||
__experimentalRole: 'content', | ||
}, | ||
mediaType: { | ||
type: 'string', | ||
__experimentalRole: 'content', | ||
}, | ||
}; | ||
|
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.
Since the last version of the Media & Text block, we've introduced __experimentalRole
properties for several attributes. The new v6Attributes
object was created to keep these attributes exactly as they were at the time of the deprecation.
const v6Supports = { | ||
...v4ToV5Supports, | ||
color: { | ||
gradients: true, | ||
link: true, | ||
__experimentalDefaultControls: { | ||
background: true, | ||
text: true, | ||
}, | ||
}, | ||
spacing: { | ||
margin: true, | ||
padding: true, | ||
}, | ||
typography: { | ||
fontSize: true, | ||
lineHeight: true, | ||
__experimentalFontFamily: true, | ||
__experimentalFontWeight: true, | ||
__experimentalFontStyle: true, | ||
__experimentalTextTransform: true, | ||
__experimentalTextDecoration: true, | ||
__experimentalLetterSpacing: true, | ||
__experimentalDefaultControls: { | ||
fontSize: true, | ||
}, | ||
}, | ||
}; |
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.
New block supports were added since the last deprecation so those must be included here or we'd lose those block support styles when migrating the block.
</div> | ||
); | ||
}, | ||
migrate: migrateDefaultAlign, |
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.
If the results of this deprecation match the block's saved content, we need to update the block's align attribute.
@@ -348,8 +560,11 @@ const v4 = { | |||
</div> | |||
); | |||
}, | |||
migrate: migrateDefaultAlign, |
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.
As each deprecation needs to bring the block up to the latest version we need to migrate the default alignment here too. The same goes for all the other existing deprecations. If they already migrate attributes, their migration function is composed with migrateDefaultAlign
.
@@ -1,5 +1,5 @@ | |||
<!-- wp:media-text {"mediaType":"image"} --> | |||
<div class="wp-block-media-text alignwide is-stacked-on-mobile"> | |||
<div class="wp-block-media-text is-stacked-on-mobile"> |
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.
For the primary block fixture, I've removed the alignwide
class as that was the default for the previous version. The current source HTML matches the current version of the block. A separate deprecation has been added to cover the deprecation of the default wide
alignment.
@@ -1,4 +1,4 @@ | |||
<!-- wp:media-text {"mediaType":"image","imageFill":true,"focalPoint":{"x":"0.56","y":"0.57"}} --> | |||
<!-- wp:media-text {"align":"wide","mediaType":"image","imageFill":true,"focalPoint":{"x":"0.56","y":"0.57"}} --> |
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 the result of the latest deprecation. The existing deprecation fixtures relied on the default wide
alignment. Now it is not the default, align
is set explicitly to wide
and therefore stored in the block comment delimiter.
@@ -1,4 +1,4 @@ | |||
<!-- wp:media-text {"align":"none","mediaType":"image"} --> | |||
<!-- wp:media-text {"mediaType":"image"} --> |
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.
Now none
is the default alignment, it will not be serialized into the block's comment delimiter.
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 testing really well for me, all the fixtures look correct, and the logic in the deprecations sounds solid to me. Thanks for all the extra comments explaining how it all works!
Is the final question figuring out what to do about the v6 deprecation and already migrated markup?
Thank you for taking the time to review and test this one @andrewserong 👍
Yes, new blocks added at the latest version or already migrated ones will "re-migrate" at the moment due to not fully being able to determine if the deprecation is eligible. Looking into the block parser and the deprecation flow, only the raw attributes parsed from the block content are passed to the |
Hrm, what an interesting problem. And it doesn't appear that we can copy the logic from the Would it be worth opening a separate smaller PR to propose updating the API / function signature for |
More than likely. I'll put something together for that tomorrow. I did explore approaching it from the other angle and having a list of disallowed classnames for given blocks in the determination of the custom CSS classes. In a scenario like that, the media and text block would be invalidated when it had no attribute.align value and the My concern with either approach was whether there were any potential performance impacts given the deprecation |
Good points, both sound like very reasonable concerns — we definitely want to avoid additional unnecessary parsing, and I can see that the two sets of attributes could very easily be confusing.
Happy to help review and test once you've got a PR up 👍 |
5def879
to
82da6db
Compare
82da6db
to
295ca16
Compare
#48815 has been merged, so I've rebased this one. It should be ready for a final review. |
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 testing well for me @aaronrobertshaw — I re-tested with a range of Media & Text blocks. Existing blocks kept their alignments correctly, and newly inserted blocks still default to no alignment 👍
I think this is looking good after the rebase, just left a tiny nit in the explanatory comment, but otherwise this LGTM! ✨
This is great! |
Fixes: #20941
Related:
What?
Switches the default
align
attribute value tonone
instead ofwide
for the Media & Text block.Why?
Makes the block's default alignment align (pun intended) with user expectations.
See discussion in: #20941
How?
align
attribute tonone
align
attribute for deprecated block's which would have previously defaulted towide
Deprecation Breakdown
Why can we use a deprecation to change the default alignment for this block?
The prior default
wide
value for thealign
attribute resulted in thealignwide
class being serialized within the saved block markup. When thealign
attribute is updated to default tonone
, no class is included in the markup. This makes the block's saved markup invalid and triggers the deprecation to run. The deprecation will run itsmigrate
function to explicitly set the old block'salign
value towide
.The
isEligible
check in the new deprecation will ensure only blocks relying on the default attribute value get migrated.What about individual blocks of the previous version that explicitly set their align attribute?
If a block of the previous version relied on the default
align
value, its saved markup would be invalid thus triggering a deprecation whereas if the block set an explicitalign
value, any class saved in the markup would be correct and so the saved markup would still be valid and no migration of thealign
attribute required.Why do all prior versions of the block also need to update their align attribute?
Each deprecation is responsible for bringing its version of the block up to the latest. As a result, prior versions must keep their original attribute definitions but still migrate attributes up to the latest. So as all prior versions of the block had the default
wide
alignment, if any of those deprecated blocks relied on that default, we need to explicitly set thealign
attribute towide
so that that alignment is maintained.What has changed with the block fixtures?
none
default alignment.Testing Instructions
TBA
Should be something like:
Screenshots or screencast
TBA