Allow using groups and columns inside the experimental form block#55758
Merged
Allow using groups and columns inside the experimental form block#55758
Conversation
aristath
added
[Type] Enhancement
aristath
requested review from
ajitbohra,
fabiankaegy,
juanmaguitar,
ndiego and
ryanwelcher
as code owners
aristath
changed the title
aristath
requested review from
draganescu
and removed request for
ajitbohra,
fabiankaegy,
juanmaguitar,
ndiego and
ryanwelcher
fabiankaegy
approved these changes
Nov 1, 2023
Member
fabiankaegy
left a comment
fabiankaegy
left a comment
There was a problem hiding this comment.
This is a really useful addition 🚀
Left one comment inline but looks good to me :)
|
Size Change: +210 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
aristath
force-pushed
the
add/groups-in-forms
branch
from
d8ffcb8 to
413ec7e
Compare
Contributor
|
Let me stop auto-merge. Wouldn't this PR allow us to put the Form block inside of the Form block? I have not tested this PR, so sorry if I am mistaken. Form > Group > Form My understanding is that nesting of form elements should not be allowed. |
Member
Author
|
@t-hamano you're absolutely right... Thank you for catching that! 👍 |
Contributor
|
Since there may be cases where multiple forms are inserted on a page, simply setting |
Member
Author
|
@t-hamano maybe we should have a gutenberg/packages/block-editor/src/store/selectors.js Lines 1385 to 1399 in 76ce7e0 canInsert const below that snippet 🤔
EDIT: Or maybe we could define in |
Member
|
@gziolo and I have long been talking about more strict rules for the block API in the block API tracking issue: #41236 So from my perspective that issue is not new and has little to do with the update here. I don't think it should prevent moving this forward but would of course love a solution for it :) |
|
Flaky tests detected in b977750. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6719428592
|
Contributor
|
What about using the This code is an example. |
Member
Author
|
The simplest change I can think of, is to allow values like |
Member
Author
Huh... Good call! Thank you @t-hamano! Pushed a commit implementing that filter. |
t-hamano
reviewed
Nov 1, 2023
Contributor
There was a problem hiding this comment.
In my testing, nesting forms appear to work correctly in all cases 👍
- Root > ✅ Form
- Root > Group > ✅ Form
- Root > Columns > Column > ✅ Form
- Root > Form > Group > ❌ Form
- Root > Form > Columns > Column > ❌ Form
As for filters, I think it's better to run them inside the init() function.
export const init = () => {
// Prevent adding forms inside forms.
// ...
return initBlock( { name, metadata, settings } );
};
Member
Author
Makes sense... Done. |
Member
I see the same filter allows removing Post Content and Template Part blocks from the post editor. It also prevents adding a Template Part block inside the Query Loop and inside the Post Content block which plays a similar role. I guess it's fine to reuse it here as well.
I agree it shouldn't be a blocker, but it looks like there is a working solution in place. The remaining question is how to offer a general solution for the same problem. There are many way how it can be approached, and the filter used would be one of them if we consider it an edge case. We could also offer a way to list disallowed parents/ancestors separately, or in this particular case offer something like |
Member
Yeah also you can pretty much always circumvent it via a synced pattern |
Member
Author
|
Since the concerns for this PR were addressed, I went ahead and merged it. |
draganescu
reviewed
Nov 2, 2023
| const DISALLOWED_PARENTS = [ 'core/form' ]; | ||
| addFilter( | ||
| 'blockEditor.__unstableCanInsertBlockType', | ||
| 'removeTemplatePartsFromPostTemplates', |
Contributor
There was a problem hiding this comment.
What do template parts and post templates have to do with this? :)
Contributor
There was a problem hiding this comment.
Good catch, this namespace would need to be changed to something like removeFormFromInserter 😅
Member
There was a problem hiding this comment.
Yes, something like core/block-library/preventInsertingFormIntoAnotherForm would fit better. I don't think we have ever established a good pattern for naming filters.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
What?
Allows adding columns and groups inside a form block.
This will allow us to build some more complex forms, with different layouts etc.
Why?
Because right now, forms only allow input fields one below the other, which limits our ability to build beautiful things.
How?
parentto usingancestorinblock.jsonfiles for input blockscore/columnsandcore/groupblocks in the allowed blocks for formsTesting Instructions
Testing Instructions for Keyboard
Not applicable