Cover: Avoid adding a wrapper Group block when transforming to Group, where possible #40293
Conversation
andrewserong
added
[Type] Bug
|
Note: I'll be AFK for the next couple of weeks, so I likely won't see any replies on this PR. If anyone would like to take this over and / or merge it while I'm away, feel free to! 🙇 |
|
Size Change: -237 B (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
|
This tested well for me, but added an empty |
There was a problem hiding this comment.
Thanks for addressing this one @andrewserong 👍
This tested well for me for transforming to and from Group blocks (including Row & Stack) with either flat color or gradient backgrounds.
If the Cover block contained a url attribute (so, it had some form of media as a background), then retain the existing behaviour where we defer to the Group block's default transform that adds a Group block as a wrapper.
Unfortunately, for me if I select a background image for the cover block and then attempt to transform to a Group block, I lose the image rather than the entire Cover block being wrapped in a new Group block.
Screen.Recording.2022-04-14.at.1.51.15.pm.mp4
It appears that the new transform is still used despite the isMatch correctly returning false. It might also pay for us to update the grouping related tests.
I'll dig into this in more detail next week.
Should custom background and gradient background colors be migrated as well? As far as I can tell they aren't, which is a regression from trunk I think. With preset:
With custom:
|
|
Are we still aiming to get this into 6.0? If not, I will remove from the project board. Thanks! |
I don't think this is critical for 6.0, and because it also relies on another fix to work, I think we should defer to 6.1. |
…block don't override real values from the Cover block
andrewserong
force-pushed
the
fix/cover-block-transform-to-group-block
branch
from
4dad7b3 to
6a8b661
Compare
…erve clearing out background / gradient in transform from the Group block
Thanks for adding in the fixes @glendaviesnz (and the related #40497) and for resolving the unnecessary wrapping when transformed from Group block @aaronrobertshaw! I've just given this a rebase and re-tested. @glendaviesnz it looks like in 796ca68 the background and gradient styles are now added in to the I've pushed an update in 508711d that moves the check down to
I've also added in calls to |
There was a problem hiding this comment.
This tested well for me now.
✅ Transformed range of Cover blocks with preset and custom colors and gradient to Group and back and no extra wrappers added
✅ Transformed Cover with background image and it went to a Group with nested Cover block
I wonder if it is worth adding some tests for this? packages/blocks/src/api/test/factory.js has some examples of how to test transformations - maybe it would be good to start adding some block specific ones to the block folders as we modify transformations to prevent regressions - I haven't looked closely at how easy/feasible it is to mock a specific transformation though, and not a blocker for this PR, could be a follow up if you think it is feasible.
|
Thanks for re-testing @glendaviesnz!
That's a great point, I'll do a little digging to see how feasible it is, but it'd be good to add some in, given that we have a bit of subtle logic going on here that'd be easy to accidentally break. I'll see how I go! |
Thanks again @glendaviesnz, that worked a treat, after a little digging, it wound up being fairly straightforward to add in tests for the transformations without having to do any mocking, I could just register the Cover and Group blocks via their metadata and then call I also added in a test to cover the behaviour @aaronrobertshaw added in #40602. Do you mind giving this another look when you get a chance, to confidence check the testing approach? Thanks! |
There was a problem hiding this comment.
Thanks for adding the tests, especially those covering #40602 🙇
I've given this another test run:
✅ Transforming Cover to Group works well
✅ Transforming Cover blocks with image or video backgrounds work
✅ Transforming Group to Cover works well
✅ Transforming Group block variations work e.g. Row / Stack
✅ Custom & named colors/gradients are moved appropriately between transformed blocks
✅ Unnecessary wrapping and empty attributes are avoided
✅ Unit tests are all passing
❓ I may have run into an issue when testing transforms of a Cover block with a single Group block as its child. I expected the Group block's colors to be applied to the result. When the original Cover block has a gradient background, the Group block's background color is lost. When the Cover has a flat background color, the Group's background color is honored. So I feel like this is a bit inconsistent. What do you think?
Screen.Recording.2022-05-03.at.5.58.46.pm.mp4
I also have one tiny suggestion to perhaps make the test names read a fraction easier but I'll leave that to you as it is completely optional.
|
Nice to see how easily the transform tests can be done - a good example to apply to other blocks as they are refactored. Agree with Aaron's note about the slight clarification of wording. Happy to retest once you have looked at the other potential issue he identified. |
|
Thanks for the feedback Aaron and Glen! I've done the following:
Please let me know if you can see any issues with that approach! |
There was a problem hiding this comment.
Thanks for the speedy updates @andrewserong 🚀
Everything looks to be working well. The new test is another welcome addition and the test wording reads much better to me now.
I think this is good to go 🚢
|
Change tested well for me I wonder if the transform tests would be worthy of a blog post? |
Oh, nice idea. I might write something up! |
Depends on #40497 Check that a transform matches at the time of running the transform
What?
Following on from #40212, this PR seeks to fix / improve some of the behaviour when transforming from a Cover block to a Group block.
The changes are:
urlattribute (so, it had some form of media as a background), then retain the existing behaviour where we defer to the Group block's default transform that adds a Group block as a wrapper — this way the user doesn't unexpectedly lose content.Why?
As noted in the review on that PR, when transforming from a Cover block to a Group block, the default transform in the Group block causes a Group block to be added as a wrapper. Repeatedly transforming between Group and Cover and back again results in repeatedly nesting the set of blocks in further levels of Group blocks. With this PR, in most cases, we should now be able to switch between Group -> Cover -> Group again without incurring additional nesting.
Note: this PR makes lots of assumptions about user intent — the hope is that the changes here get the behaviour closer to what users might expect when transforming from Cover to Group, but it is making assumptions, and the transform is still a destructive action, so it isn't perfect. Happy for feedback (or pushback) on the change if folks think there's a better way to go about this.
How?
Testing Instructions
Tests are also included, and can be run from a terminal via:
Screenshots or screencast