Add align support to the image block #55346
Conversation
|
Size Change: +3.21 kB (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
|
Any idea why this wasn't implemented via the P.S. I would avoid fixing the ESLint hook warning in the same PR. Ones in the media blocks are fragile and can have unexpected side effects. |
|
Yes, I have no idea! Maybe it's a relic? My goal with this is to see what breaks 😂 |
|
@richtabor, I was mostly asking why current alignment controls are hard-coded for the block instead of using |
|
I am stuck on regenerating snapshots on my env. |
|
I tried to dig into the original reason a while back and documented my findings here: #19103 (comment) |
draganescu
added
[Type] Code Quality
draganescu
force-pushed
the
add/align-support-image-block
branch
from
74f5579 to
01dd55b
Compare
draganescu
requested review from
artemiomorales,
michalczaplinski,
ajitbohra,
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
draganescu
removed request for
juanmaguitar,
ajitbohra,
ryanwelcher,
ndiego,
artemiomorales and
michalczaplinski
There was a problem hiding this comment.
@draganescu, it's probably better to have changes related to the enhancement and revert ESList fixes - #55346 (comment).
| const extraUpdatedAttributes = [ 'wide', 'full' ].includes( nextAlign ) | ||
| ? { | ||
| width: undefined, | ||
| height: undefined, | ||
| aspectRatio: undefined, | ||
| scale: undefined, | ||
| } | ||
| : {}; |
There was a problem hiding this comment.
As @fabiankaegy mentioned, resetting these values seems to be the one custom thing the image block did when changing alignments, and it should probably be retained.
An effect would be one way to handle it, the downside being modifying alignment and resetting the size become two separate changes on the undo stack.
Quietly whispered: Filtering alignment attribute changes could also work to avoid the undo issue.
Not sure if there are any other options, would be good to hear some ideas.
There was a problem hiding this comment.
The "Quietly whispered" means maybe adding a new filter to where alignment is updated by the block support?
There was a problem hiding this comment.
Would it work to still handle it in an effect but using the markNextChangeAsNonPersistent method to not create anything in the undo stack for the change?
Not sure whether the change would properly get reverted when someone does go back one step at that point. I just know we have something for that
draganescu
force-pushed
the
add/align-support-image-block
branch
from
01dd55b to
3eafa16
Compare
|
I removed the commits that updated the ESLinit complaints and left only one commit removing the custom alignment. |
|
In 3a5aaac i use an effect to reset width, height, aspect ratio and cover. In d88c9c3 I use a filter in the align hook which the image block then uses to reset width, height, aspect ratio and cover when align is certain values. I think the second path is better than |
|
Flaky tests detected in ba39269. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6768476257
|
There was a problem hiding this comment.
This seems to work well in my testing.
Wide alignment in a theme with one column ✅
Full alignment in a theme with one column ✅
Left alignment in a theme with two columns ✅
Centre alignment in a theme with two columns ✅
Right alignment in a theme with two columns ✅
| const filteredAttributes = applyFilters( | ||
| 'block-library.image.alignmentUpdate', | ||
| blockName, | ||
| { ...attributes, align: nextAlign } |
There was a problem hiding this comment.
Looking at the way the UPDATE_BLOCK_ATTRIBUTES reducer works, I think it might be best to keep the object here restricted to only the attributes that are being modified rather than all attributes.
Passing all causes an unnecessary performance hit, as the code loops through the attributes one by one.
Could still provide all attributes as a separate param as it was before, so that the filter function can make decisions based on the values.
|
@talldan @ajlende @fabiankaegy I opened #55954 as a simpler alternative. I for one like the idea of as many filters as possible, and also I am not sure there is a significant performance concern of these filters existing. Any filter can be used in the wrong way, but particularly those who get called only when a certain action is performed on one single block at time are unlikely imo to cause bottlenecks. But I may be wrong. I am fine with going the other route as well since the goal here is to make the image block better not to debate the high level idea of where should filters live and how many should we place in the runtime. 🙏🏻 |
I don't have strongly held opinions on this, but just want to share my thought processes when suggesting a filter. Effects are used in some places in blocks, but usually as a trade-off or because there's no good alternative.
I'm not a huge fan of filters, but in this case I think it might be ok to avoid a double call of |
These are some good points, and I'm not fully opposed to adding new public APIs to solve problems, but I want to make sure we're thinking holistically about the project when we add them. To expand on what I was saying earlier, the problem of needing to modify |
|
@ajlende would this mean to try a global filter that can run everytime setAttributes is called? |
|
@draganescu is this PR still relevant given #55954 got merged? |
|
@getdave yes, this is the better way - I'll try to see if it eventually replaces the other solution. |
|
I haven't given any more attention to this so I will close it as I also haven't seen any issues or complaints around the solution that has been merged. Let's leave it for later. |
Closes #19103
This PR removes the custom align control from the image block and makes the block support align controls.
From the tests we run the only issue is the expected snapshot problem, nothing else.
Approach
This PR proposes a new filter on the align support behavior. This approach would allow other blocks that support align to filter the behavior when it sets "wide" or "full", which are opinionated values that may lead to specific collateral uodates - like it is the case with the image block.
The downsides of this filter are:
In #55346 a simpler alternative approach is explored where we limit the resetting to the image block and make sure there is only one undo step which undoes both the align setting and the other attributes resetting via
__unstableMarkNextChangeAsNotPersistent.