Cover: Always allow transform from Group block, add handling for Row and Stack variations #40212
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
…and Stack variations
andrewserong
added
[Block] Cover
|
Size Change: +2.23 kB (0%) Total Size: 1.22 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks for putting this together @andrewserong. These transforms look pretty handy for end users. 👍
My initial testing went as follows:
GroupTransforms.mp4
For a standard Group block, this works great! I had slightly more mixed results when testing with the Row or Stack variations.
If they had a flat background color the resulting Cover block looked as I'd have expected (with the very minor exception of some squashing of Row inner blocks).
When they have a gradient background, we get a full gradient overlay for the Cover block but the wrapped Group block variation doesn't fill the Cover block. This looks a bit odd and I'm not sure how easy or intuitive it will be for users to correct.
Once we've detected a flex layout variant, could we tweak more in the way of layout attributes to improve this?
Screen.Recording.2022-04-11.at.8.16.23.pm.mp4
Also, once you transform a Group to a Cover block, going back to the transforms menu again, it appears like you can convert "back" to the original Group block. Performing this transform repeatedly results in nested Cover blocks.
Is it feasible for us to setup transforms back in the other direction?
I don't believe any of the above is critical if you'd prefer to explore refining these aspects separately.
ntsekouras
reviewed
Apr 11, 2022
| }; | ||
|
|
||
| // For variations that use a flex layout (e.g. Row and Stack), | ||
| // wrap the block in a Cover instead of converting directly to the cover block. |
There was a problem hiding this comment.
I think some rewording like preserve the Group block or something might be a bit more clear.
Also I'm wondering if we should be consistent here and always add the Group block nested in the new Cover block. That way we can keep the other layout settings.
Finally I think it makes sense to remove existing background related attributes, if they exist, from the Group, as they are also added to the Cover. For example if we have a Group with background red, it will result in both Group and Cover to have the red background. Makes sense?
…block for all variations
|
Thanks for the feedback @aaronrobertshaw and @ntsekouras! I think by doing the following I've addressed most of the issues, but let me know if I missed anything 🙂
I started having a look at this, too, but it also involves making a few decisions about when to treat the inner group as the block to transform to, and when to preserve it. Since I'll be going AFK after tomorrow, I might look into that in a separate PR so as not to hold up getting this fix in. (That way I can leave it as a WIP if it isn't ready before I go AFK). Thanks again! |
aaronrobertshaw
approved these changes
Apr 12, 2022
There was a problem hiding this comment.
Nice work and quick turnaround on these fixes 🚀
Take background color or gradient settings from the Group block, and apply to the parent Cover block in the transformation, remove from the Group block.
This fixes the issue in the resulting Cover block for Group's with a gradient background. 👍
(Seems to fix the padding issue on the Row block, too)
Removing the background from the wrapped Group block does remove the default padding that is applied to Group blocks with the has-background class.
I think that might be all we can do. Some blocks, e.g. Buttons, within a Row block will eat up excessive space and possibly cause some wrapping.
In all cases nest the Group block within the Cover block so that layout is preserved.
✅ When converting a Group, Row, or Stack, they are all preserved within the newly created Cover block.
I might look into that in a separate PR
Sounds like a plan. With that in mind, I'd say this is good to go 🚢
Screen.Recording.2022-04-12.at.4.02.51.pm.mp4
andrewserong
added
the
Backport to WP 6.7 Beta/RC
|
Thanks for reviewing, Aaron! 🙇 |
andrewserong
deleted the
update/group-to-cover-block-transformation-remove-conditions
branch
|
I've opened a follow-up PR to explore going from Cover -> Group in #40293 — I'm about to head off on leave, so I likely won't see any replies on it, but anyone's welcome to take over that PR if they'd like to. Otherwise, I'll be happy to keep chipping away in a couple of weeks! |
adamziel
pushed a commit
that referenced
this pull request
Apr 13, 2022
…and Stack variations (#40212) * Cover: Always allow transform from Group block, add handling for Row and Stack variations * Move background color or gradient settings to parent, preserve Group block for all variations
adamziel
removed
the
Backport to WP 6.7 Beta/RC
|
This was cherry-picked to the |
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.
Fixes #40209
What?
As raised in #40209, remove the condition that the transform from a Group block to a Cover block option is conditional on a background color being present. Also, add handling to preserve the Stack and Row variations by wrapping those blocks in a Cover block, instead of converting directly to the cover block.
Why?
Without this option, users wishing to move blocks from a Group to a Cover block need to copy / paste / drag blocks around, so this should make it a little easier to transition a set of blocks over to the Cover block.
How?
dimRatio) of50%if there is no background color — this is needed because the default background is black, and if we otherwise set opacity to0then it would not be obvious how to get the color to appear once a user does add a color. Let me know if you can think of a better default other than this medium gray (it's consistent with the settings for if someone removes an image from a cover block).Testing Instructions
Screenshots or screencast
Note: I've used the Bug label as this feels like a UX issue, but could arguably be an enhancement.