Add align support to the image block - alternative #55954
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
draganescu
changed the title
draganescu
added
[Type] Code Quality
|
Size Change: -37 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
draganescu
requested review from
ellatrix,
artemiomorales,
michalczaplinski,
ajitbohra,
ryanwelcher,
juanmaguitar and
ndiego
as code owners
ajlende
approved these changes
Nov 8, 2023
There was a problem hiding this comment.
I made one change of removing the custom align attribute and allowing the block supports to add it.
Tested both the image block and the gallery block, and everything seems to work.
I'm not sure of the purpose of align support within the context of the gallery, but the behavior matches trunk.
Comment on lines
-12
to
-14
| "align": { | ||
| "type": "string" | ||
| }, |
There was a problem hiding this comment.
I went ahead and added a commit to remove this since it gets added by the block supports. The only difference should be the serialization order moving the align attribute to the block support section after the custom attribute section.
| @@ -19,25 +19,25 @@ exports[`Align options for group block sets Wide width option 1`] = ` | |||
| `; | |||
|
|
|||
| exports[`Align options for media block sets Align center option 1`] = ` | |||
| "<!-- wp:image {"align":"center","id":1,"sizeSlug":"large","linkDestination":"none"} --> | |||
There was a problem hiding this comment.
Technically this test was incorrect before as it was testing the image block's align implementation instead of the hook, so it's nice that it's testing the correct thing now.
|
@talldan since the discussion around adding a filter is complex in regard to where the filter is best suited - although your idea to add a filter for issues like #19103 is better than this PR - I suggets we land an effect to close the issue and come back to the filter when we develop a better, more holistic understanding. It's easy to remove this effect when we have a better, more universal solution. What do you think? |
|
👍 Sure, go for it. This is testing well for me, so no issues with merging it. |
|
This tests well for me. I was specifically looking for potential issues when the Lightbox is enabled but didn't find any 👍 |
…gn is wide or full
This allows the block supports to add the attribute instead keeping things cleaner.
draganescu
force-pushed
the
add/align-support-image-block-skip-undo-no-filter
branch
from
5ee9aa7
to
e5c65b6
Compare
draganescu
deleted the
add/align-support-image-block-skip-undo-no-filter
branch
|
I think on principle I agree with this change. But there are a few things that we should be very careful about here. I think image blocks work differently between very old classic themes (no layout support, no theme.json...) and new ones, so we should make sure that there's no impact there editor and frontend. The other thing I noticed is that the "align" support is responsible for adding the classnames to both frontend and editor, this PR however doesn't remove the custom code that we have in save and edit functions of the image block that does the same. Does that mean we'll end up with duplicate classnames? Was that accounted for differently? |
|
Ha, I think the support overwrites the classname in the save function. I'll do a PR to remove the save code as it's not needed anymore. In my testing removing that does not have any effect. |
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.
Alternative to #55346
Closes #19103
This PR removes the custom align control from the image block and makes the block support align controls.
Approach
This PR proposes resets the image aspect ratio, dimentions and cover attributes via an effect when the align attribute is updated to be one of
wideorfull.The downsides of this approach are:
__unstableMarkNextChangeAsNotPersistentto get out of the trouble with using effects :) I think since we keep using this maybe we should have a history api that allows us to sayhistory.pause()andhistory.resume().In #55346 a more complicated alternative is explored which proposes a new filter on the align support behavior which would open the possibility to change how any block that suports align wide or full does collateral attribute updates.