Suggest Pattern transformations that are contextual to the currently selected blocks #29890
Conversation
ntsekouras
requested review from
mtias,
jameskoster,
kjellr,
jasmussen and
a team
|
Size Change: +1.53 kB (0%) Total Size: 1.42 MB
ℹ️ View Unchanged
|
|
So cool. I tested this as is without any additional patterns registered. Keeping that in mind, I'm seeing some duplication in the menu:
And some tall stuff here:
But the principle is cool: the text I transform is applied inside the pattern, the added stuff just lands also. I think for some blocks, like galleries, cover, media & text and columns, this feature will be a big help. There are also some menu item polish tweaks that need to happen, happy to help here. But as a proof of concept, even now it feels promising. It will also be useful, I suspect, for a different paginated view that Jay and I have been discussing. |
This seems like a very sensible way to approach this for now 👍 One small potential issue I spotted: If you transform some blocks to a pattern, ungroup the blocks, reselect them all, open the transform menu, I don't see the patterns menu. Should I? Video to explain: pattern.mp4I suppose perhaps not, since the blocks already match the pattern. Another problem which seems to be a bug; if you delete one of the blocks from a transformed pattern, select those that remain and open the pattern menu and try to select a pattern, it seems to fail, and eventually crash: re-pattern.mp4I don't know if this qualifies as a "design detail" you asked us to ignore, but is the plan to invoke the carousel/grid ui from #28736 (comment) down the road? |
It's not duplicated by fault, I have the same content in two patterns with the difference that one has one more heading (
Appreciate your offer to help and will take it :). I'll ping you when the time comes.
@jameskoster that's not a detail. I think this is a design decision that needs to be made and then I'll change accordingly the code. Of course we will need to first land the PR that introduces the carousel: #29602. I'll check the bugs you mention above. Thanks for testing both! |
|
I tested with a theme that does not register any patterns, so the default patterns are used. |
ntsekouras
added
[Feature] Patterns
vindl
requested review from
mattwiebe,
jeyip,
vindl,
Addison-Stavlo,
Copons,
creativecoder and
david-szabo97
|
This is excellent! Super excited to see this coming through. |
ntsekouras
force-pushed
the
try/patterns-transforms
branch
from
d3f7208 to
5ecd95b
Compare
This happens because in my test pattern the paragraph is also in a
This is not handled - the check if selected blocks are identical in terms of content/attributes/InnerBlocks with pattern blocks.
This was a bug with the cloning and now is fixed.
Do anyone have any thoughts on that? Because if that's the case we should focus first on landing the PR that introduces the carousel: #29602. -cc @jameskoster, @mtias , @jasmussen , @mcsf , @Addison-Stavlo |
|
@ntsekouras How do you ensure the "content" is preserved and not replace with the pattern's attributes? |
I merge the attributes of the selected block and the matched pattern block and prioritize the selected block's attributes. Regarding blocks with InnerBlocks like That was what you were asking, no? |
|
yes, this answers my questions, it does sound a bit arbitrary though and a bit hard to reason about. For instance say I have a simple unstyled paragraph block and two patterns that have some random content and different background colors:
If I do the opposite (start with pattern B), I'll get a different result. That said, I'm not sure I can come up with a good algorithm to be honest, it sounds too specific to the pattern itself that it's hard to come up with the right transformation in a generic way. |
Exactly! That is something I'm still thinking about and I'm not sure either how it should be handled the best. I don't think there is an easy way to be too smart and more importantly if we should try to.. |
I'm fine going with this one separately. The carousel has a place, but needs a bit more work in my mind, and a simpler transform for patterns will still be needed, I think. |
|
I tested this out a bit and it is a really neat idea that I think could become a very powerful tool for the block editor! 🎉
I feel like the carousel layout may make a bit more sense for Template Parts specifically. Here they feel a little bit out of place as they behave a bit differently from the other flows with selections of normal blocks: they are limited to single selection, do not preserve content, and do not surface patterns contextually based off of the inner blocks they contain. Those behaviors make sense for template parts, but they are also inconsistent with the flows with standard blocks. I feel like the carousel / placeholder pattern would fit better for these behaviors, and prefer a different UI for a different behavioral flow (as opposed to reusing the same UI across a different behavioral flow resulting in the inconsistent feel in UX).
@mtias - Im curious about how the carousel would need a lot more work than this. From a technical standpoint, it seems like it would be much less complex: determining which patterns to show in that circumstance is quite simple and it has no requirement for preserving content. It seems like it would make a lot of sense for Template Parts specifically, and if we can limit it to that block at first we can get user feedback from a more forgiving beta environment that is currently undergoing a series of testing cycles. |
|
Jays feedback works well here:
Note that tall heading is my own fault:
It feels a little weird that we have "Transform to" as a subheading on the 2nd group, when a pattern is also a transform:
@jameskoster is there a design where block transformations and pattern transformations coexist in the same section? Or at least share the same heading? I pushed some polish to paddings and hover styles:
|
ntsekouras
force-pushed
the
try/patterns-transforms
branch
from
3eee2e5 to
f8e8e76
Compare
@Addison-Stavlo It needs more work from a design perspective. We cannot use the block canvas carousel in all contexts due to space constrains, so we need to explore a way to work well with that — either isolated template part or modal. Also the double toolbar aspect is tricky. It's also not super clear yet that it's a selection step and not just the content of the block. I think we need all of that in a good place to try with template parts. |
ntsekouras
force-pushed
the
try/patterns-transforms
branch
from
5fcf92a to
5f64abb
Compare
ntsekouras
force-pushed
the
try/patterns-transforms
branch
from
48d2ff5 to
796fe41
Compare
ntsekouras
force-pushed
the
try/patterns-transforms
branch
from
796fe41 to
8bc5a9f
Compare
There was a problem hiding this comment.
This is some really cool work. There are lots of little pieces to unpack, so it took me a while to review.
First of all, don't pay too much attention to my inline comments: they tend to be micro-level observations.
As for the overall approach:
- Personally I'm on board with the
"role": "content"trait in the block schema, and letting the transforms system make decisions based on that. - I like the focus on the user that shines through. You're clearly exploring different methods of transformations to see what would offer the most value.
- However, that comes at the cost of complexity and arbitrariness in our components (e.g.
replaceInnerBlocksMode,blocksToSkip). - There are enough parallels and distinctions between "regular" block-wise transforms and pattern-wise transforms that we need to ponder whether to consolidate the two, or whether to set them clearly apart. Right now, pattern-wise transforms are piggybacking off block-wise transforms.
- A lot of juggling is being done between state selectors and different layers of UI components, which makes it very hard to reason about the data flowing through.
Given some of the issues raised above, is it possible to break apart some of this work so that:
- The first PR only does one kind of transformation (i.e. no
replaceInnerBlocksMode) and tries to do it as neatly as possible, based on `"role": "content", and focusing on the data flow. - Different kinds of transformations can then be explored each in its own PR (e.g. the special handling of Template Parts). Similarly, we can debate any introduction of "magic" in dedicated PRs.
- Bonus: can we measure the performance of opening the block switcher against
trunk?
| // If a selected block is nested (with InnerBlocks like Group or Columns) | ||
| // return an EMPTY_ARRAY, as it doesn't make sense to try to be too smart. | ||
| // In such blocks a user could have anything inside as content, so don't show | ||
| // any transforms. In these blocks it only makes sense to show `block` scoped | ||
| // patterns during insertion, where no content exists yet. |
There was a problem hiding this comment.
What happens with other container blocks? What's the difference with just checking for inner blocks?
if (blocks.some(b => b.innerBlocks.length)) return EMPTY_ARRAY;And do we need to treat certain containers differently? For example (and this isn't meant to further block/complicate this PR!) what might change once the Quote block supports nested content? Can the role: 'content' concept help in those cases?
There was a problem hiding this comment.
The difference between such blocks is that some of them are mean more as grouping blocks. Examples are Group, Columns, Template Part and:
// In such blocks a user could have anything inside as content, so don't show
// any transforms. In these blocks it only makes sense to showblockscoped
// patterns during insertion, where no content exists yet.
This could work differently in simpler(more semantic) blocks with InnerBlocks like Buttons, Quote, Cover. While the user could still have anything inside it's still more targeted to specific semantics and the implementation at this point kept all the selected block's InnerBlocks.
This is a great point that I wasn't sure and wanted feedback and maybe it should be handled not by the framework but instead be explicitly declared in the block somewhere(?)
| _patterns = statePatterns.map( ( statePattern ) => ( { | ||
| ...statePattern, | ||
| transformedBlocks: statePattern.blocks.map( ( block ) => | ||
| cloneBlock( block ) | ||
| ), |
There was a problem hiding this comment.
This is one of the crucial spots, for me, where it becomes apparent that these different data structures moving around become confusing: WPEditorTransformItem, { ...WPEditorTransform, transformedBlocks }, plus some of the bindings (transformedBlocks -> patternBlocks`).
There was a problem hiding this comment.
WPEditorTransformItem this is wrong type from jsdoc paste :)
packages/block-editor/src/components/block-switcher/pattern-transformations-menu.js
Show resolved
Hide resolved
| // We have found a matched block type, so | ||
| // add it to the transformed blocks Set and return it. | ||
| transformedBlocks.add( clientId ); |
There was a problem hiding this comment.
Correct me if I'm wrong, but I don't think we should mutate this set inside this function. It's probably up to the consumer of that function to provide the right set of excluded block names. Since the consumer will probably be an iterative algorithm, it will roughly look like this:
consumedBlocks := Set()
for patternBlock in patternBlocksToProcess:
matchingBlock := findMatchingBlockInPattern(
patternBlock,
selectedBlockName,
consumedBlocks)
if matchingBlock:
consumedBlocks.add(matchingBlock.clientId)
| // Recurse through every pattern block | ||
| // to find matches with each selected block, | ||
| // and transform these blocks (we mutate patternBlocks). |
There was a problem hiding this comment.
we mutate patternBlocks
I can see why we would prefer to mutate in this case: it can be a lot of data to duplicate, a lot of allocation, and the input is already the result of cloneBlocks, so we might as well transform it in place. However, we should extract all of this logic so that it's clear where the boundaries of data mutation are. IMO, the body of the useMemo call should be much smaller and just call a couple of well tested helpers — I do realise that your request for feedback was about the whole approach and not these implementation details, and you were probably going to streamline this module anyway. :)
There was a problem hiding this comment.
I do realise that your request for feedback was about the whole approach and not these implementation details, and you were probably going to streamline this module anyway. :)
Exactly, although your comments are helpful :)
|
Thanks for reviewing @mcsf ! I hear your concerns about the complexity and arbitrariness and this is indeed a complex problem to find the right balance between magic and APIs.
I'll spin off a new PR for that. |
…ently selected 'simple' blocks (no InnerBlocks) (#30469) * spin off from #29890 for simple blocks without InnerBlocks * __experimentalGetBlockAttributesNamesByRole tests * update patterns * useTransformedPatterns and refactoring part 1 * getMatchingBlockByName tests + and other jsdoc * getPatternTransformedBlocks + transformMatchingBlock tests * add role:content to more blocks + a couple test patterns * pattern list padding * rename role to __experimentalRole * Update docs suggestion Co-authored-by: Miguel Fonseca <[email protected]> * fix tests + docs * change patterns * update social pattern Co-authored-by: James Koster <[email protected]> Co-authored-by: Miguel Fonseca <[email protected]>
Description
Resolves: #28736
Part of: #27575
This is a POC for suggesting block pattern transformations that are contextual to the currently selected blocks.
First of all we need to decide what type of transformations we will suggest/support and we have to keep in mind flows for template parts and any other special cases we can think of. You can find discussions on the above issues and especially here.
Rationale behind the current decisions
grouping blocks(likeGrouporColumns) during insertion inPlaceholder(explored here), it doesn't make sense to suggest any pattern to transform, as a user could have anything inside as content. So if any selected block falls into this category, do not suggest any transforms.paragraphselected and show patterns that do not includeparagraphbut have transformed theparagraphto aquoteblock.Buttons, keep only the blocks from the selected block and skip the inner blocks from the pattern. You can test this with a testButtonspattern I have in this PR.I have many comments in my code trying to explain as good as I can the flows.
Testing instructions
lib/test-block-patterns.phpor register one yourselvesScreenshots
Notes
I would love your thoughts and feedback on this!
Checklist: