Accordion block: experiment with the concept of a 'composite' container block - #73510
Accordion block: experiment with the concept of a 'composite' container block#73510talldan wants to merge 8 commits into
Conversation
130a1ad to
b97573e
Compare
|
Size Change: -7 B (0%) Total Size: 2.54 MB
ℹ️ View Unchanged
|
| expect( canRemoveBlock( state, 'block1' ) ).toBe( false ); | ||
| } ); | ||
|
|
||
| it( 'denies removal when parent has templateLock set to all', () => { |
There was a problem hiding this comment.
Claude did add tests for stuff not introduced in this PR, but I'm not complaining, the tests look good to me and broadens the code coverage.
talldan
added
[Type] Enhancement
21ba1eb to
c9887e1
Compare
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
This is working well for me: Kapture.2025-11-26.at.14.39.50.mp4I think it's a good base for iteration, but would prefer to wait for @t-hamano's advice. Just so I understand, with the composite block API, is the intention that Also, Claude flagged the following edge cases - is it worth writing unit tests for these? // Composite + block lock attributes: What if a child of a composite block has lock.remove: false or lock.move: false? Does composite take precedence?
it( 'composite parent prevents removal even when child has lock.remove set to false', () => {
// Test that composite overrides lock.remove: false
} );
// Composite + templateLock: What if a composite block also has templateLock: 'contentOnly' or templateLock: 'insert'? Does composite take precedence?
it( 'composite parent prevents removal even when templateLock is insert', () => {
// Test that composite overrides templateLock: 'insert'
} );Happy to add these tests, but just wanted to be sure of the implementation |
|
Thanks for the PR! Personally, I'm a bit hesitant to add yet another API, especially since two related APIs, templateLock and contentRole, are already exposed 😅 In the future, it certainly might make sense to add new APIs for composite components, but for now, I prefer to investigate why the parent's For example, I'm wondering if we can make a small change here to prioritize this setting. const _templateLock =
// Inherit the parent lock only when the child does not explicitly define a template lock
templateLock === undefined ? parentLock : templateLock;That said, the API this PR adds is private, so if no other ideal solution exists, I think it's OK to go ahead with this PR. |
Fair point. Let's see what other folks think before rushing in. Are there alternatives to fixing this issue? |
@t-hamano It is documented that this is how I'm not sure how to change that without a back compat break. There are also quite a few UX considerations if some parts of a That's also only one of the 3 issues mentioned. Continuing to use |
ramonjd
left a comment
ramonjd
left a comment
There was a problem hiding this comment.
For what it's worth, this is working well for me.
The icons don't appear in a "templateLock":"contentOnly" group. I can add/remove paragraphs in the panel too, but I can't remove the accordion heading/panel
With grouping and locking:
This solves a few bugs - could we merge and see how it goes?
I had a couple of questions about extra edge case test coverage here: #73510 (comment)
I wasn't sure whether it was overkill.
I can add the tests, I also don't mind if you commit that kind of thing directly to the PR, seeing as you were already half way there. Claude mostly wrote the tests for me in the first place, so it's funny that your Claude mentioned some missing cases.
I don't think there's any back compat concerns with removing |
No worries, thanks for the explainer. LGTM |
youknowriad
left a comment
youknowriad
left a comment
There was a problem hiding this comment.
Can I have a summary of what this "composite" is supposed to do. I think we should be very intentional when introducing new things to the block API and I want to make sure we're not adding something new to fix an edge case that might be solved differently.
|
@youknowriad The long-term aim is a way to build a single block that's composed of other inner blocks. 'Accordion Item' is already trying to do this in trunk (the idea is that it's inner 'Accordion Heading' and 'Accordion Panel' blocks are present by default and should never be removable). The code in trunk tries to achieve this by hard-coding
|
For me that's what simplified pattern editing is about, so I'm a bit concerned about duplication here. |
My understanding is the that intent and scope are different between composite blocks and pattern editing, probably with some nuance that I'm missing.
I think I see the direction you're going: both systems do deal with "restricting what users can modify in composite structures", so there's overlap in the problem space. I'm just wondering if the problems aren't at different architectural levels. E.g., When editing a pattern in "Pattern editing" mode, there's an Accordion block inside that is permanently in "pattern editing" mode? I guess if "content only" could permanently be applied to section, but then you'd inevitably run into exceptions for blocks that want to be "special". For this PR specifically, it also addresses technical issues that pattern editing doesn't solve, particularly around templateLock inheritance bugs and misleading UI affordances, though @danr can check my assumptions here. Anyway, I'm just assuming a lot, curious to hear what others think. |
|
If I'm reading properly, you're saying that (correct me if I'm wrong)
For me that is very similar, and I wonder if this just indicates that we're lacking "tools/settings" on the pattern building UI to achieve the outcome that you're reaching for with this "composite mode". At least that's how I see it conceptually. I do acknowledge that a developer way to "compose" a block out of "multiple blocks" or "multiple block areas" separated by random content has been requested for some time and could be addressed API wise but I feel like a decision has been made a long time ago that "patterns" are the solution to that. Now, I may be wrong but I'll defer to @mtias here. |
In a way, but not exactly. Yes, composite block is for developers to build blocks with the exact restrictions they require universally. For me it's a bit more than being about simplifying the editing experience. So a user would enter a block in the editor independent of any pattern, and that block's UX is constrained in the way the developer wants, not how pattern editing mode dictates. That's just me. I know you have more context on these things so I trust your opinion!
Good question. I'm not sure, maybe it's even something we don't need to solve right now. |
From memory I think Matías might have proposed the composite block idea originally. It does serve a similar goal of simplification (to the user) as pattern/content editing, but locked into how blocks are developed, rather than how they're composed in patterns. I'm aware I'm basically just paraphrasing how @ramonjd already put it above (#73510 (comment)) 😄 I agree to be cautious about introducing new APIs, but in this case, it does feel like a pretty common use case. Beyond the accordion blocks in core, I'm aware that there are plugins looking for a feature like this (e.g. in the Jetpack plugin, a composite blocks feature could help simplify the UI for form fields that are constructed as a controlled set of blocks out of a label and an input block, for example). In these kinds of cases, I believe there should be an API to handle this, rather than depending on templateLock, for the reasons this PR proposes. |
|
It could use the pattern editing concept, but I think there's quite a few modifications that would be required to the regular experience:
There's currently no way to make a block a section from its config, so this may require a new API anyway (though we could also use that API on template parts / synced patterns). I think it also depends on the priority of those bugs, whether they should be fixed sooner or we're happy to wait for these features to be built. |
|
Closing the PR as there seems to be disagreement, and this problem area is not where my focus is right now. |
5 participants
What?
Attempts a simple first iteration of #70994, using the Accordion Item block. The accordion block previously used templateLock as a way to compose blocks in this structure:
templateLock: all)templateLock: false)But as noted below, the use of
templateLockcauses some bugs.Why?
It was noted in the discussion here that there are currently 2-3 bugs with the accordion block:
core/accordion-panelblock explicitly setstemplateLock: falsein its block attributes, and this unexpectedly overrides any templateLock set by an ancestor.contentOnlyinsertion feature was interfering with the accordion block allowing extracore/accordion-headerandcore/accordion-panelblocks to be inserted into acore/accordion-item.core/accordion-headerandcore/accordion-paneldisplay lock icons, but the user can't unlock them, so that can be misleading.There might be ways to work around these problems, but this type of block composition is what #70994 proposes to solve, so I think it might be an idea to ship an early iteration.
How?
Adds a (private for now)
compositeflag to thecore/accordion-itemblock. The flag indicates to the editor that all direct children of this block are fixed. While they can still be edited, they cannot be moved or removed, and additional blocks cannot be inserted into a composite container. The idea is to use this flag with atemplate. It's a separate API totemplateLockso that it doesn't interfere withtemplateLock's inheritance rules. Another reason to keep it separate is thattemplateLockcan be modified by users, and this should be something not modifiable by users.Also adds
supports.inserter = falsetocore/accordion-headerandcore/accordion-panelso that they don't ever appear in the inserter.Testing Instructions
tempateLock: 'contentOnly'
Block locking
Screenshots or screencast
Kapture.2025-11-25.at.14.54.03.mp4