[Block: Group] Add transform to row variation #36202
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
carolinan
added
[Block] Group
|
Size Change: +1.66 kB (0%) Total Size: 1.14 MB
ℹ️ View Unchanged
|
carolinan
changed the title
Mamaduka
force-pushed
the
update/group-transform-to-variation
branch
from
f46a4d5
to
88f7536
Compare
Mamaduka
reviewed
Jan 4, 2022
Co-authored-by: George Mamadashvili <[email protected]>
Mamaduka
force-pushed
the
update/group-transform-to-variation
branch
from
c1a0fce
to
bc08272
Compare
youknowriad
reviewed
Feb 7, 2022
| attributes: { layout: { type: 'default' } }, | ||
| scope: [ 'transform' ], | ||
| isActive: ( blockAttributes ) => | ||
| blockAttributes.layout?.type === 'default', |
There was a problem hiding this comment.
When we insert a group block, it's not checked because an undefined layout is also a "group" block. So we should add ! blockAttributes.layout || blockAttributes.layout?.type === 'default' here
youknowriad
reviewed
Feb 7, 2022
@ndiego had similar questions a few ago, this is the explanation I could find in GitHub archives - #25231 (comment) |
Yep that's been brought up a couple of times, the chief argument being that the difference between the two transforms is a bit hard to understand. The details are fuzzy, but I vaguely recall the challenge being related to getting the mover control for child blocks to switch orientation with the direction, leading this to needing its own separate interface. But if we can combine the two, it makes sense to me, as transforms are so powerful. Transforms in general need some design explorations, it can become an exceptionally powerful interface and one that you intuitively want to check for each block, as it can contain a multitude of apperance related properties. I'm not sure whether such a design exploration needs to happen before the two interfaces can be merged, but an exploration should definitely take it into account. As a provocation (and not a strong opinion at all), I want to ask a different question: what if Group was a single block with the Layout orientation switcher? This one: The question is: is the direction of the content a transform or a property? I understand it is in the technical sense, since Group isn't a flex container. But conceptually it's worth thinking about why the interface is different. |
I think that's totally possible, for me the main "pro" of having a different variation is surfacing a "row" in the inserter. Once inserter it matters less whether you should switch on the sidebar or using a variation transform. |
Absolutely. It's also a good term to use in conversation around site building. Assuming it's possible, it would be good for any design exercises to consider whether we then need both the direction switcher, and the transforms interface between the two, at least in terms of managing prominence. |
|
@jasmussen I also wanted to clarify that switching between "flex" and "default" layout is not a "direction" switching even if it looks like that. It has more impact than that:
For this reason and even if we keep a layout switcher in the block inspector directly, I'm not sure it should be advertised as "direction". |
Not suggesting the following, but just in terms of understanding:
|
Not anymore 😄 |
|
In some ways, I kinda wish the Row block was it's own block and not a variation of the Group block. It is actually quite different and then a normal block transformation could be done between the two. For example, the "Inherit default layout" selector exists on Row blocks, but shouldn't since it's a flex container. Also, when that setting is enabled it turns the block back into a normal Group block. I think the ship has sailed on this, but having the Row block be its own block would have probably been the easiest scenario. |
|
@jasmussen Indeed we do have "orientation" support in the "flex" layout directly. So we have:
|
Right, so "Row" could have the orientation switcher as a way to get to the "Column", perhaps? Just thinking out loud at this point, and it'd generally seem best to abstract the terminology away since they're all just containers with properties. |
|
@jasmussen yes, the orientation switcher is there but hidden :P |
|
To avoid blocking this PR which is indeed a nice feature. Can we do this:
|
aristath
approved these changes
Feb 8, 2022
This was referenced Feb 24, 2022
This was referenced Apr 14, 2022
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.
Description
Makes it possible to transform a group into a row variation and vice versa.
For #35355
How has this been tested?
Screenshots
Types of changes
Enhancement
Checklist:
*.native.jsfiles for terms that need renaming or removal).