-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow using groups and columns inside the experimental form block #55758
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
|
d8ffcb8
to
413ec7e
Compare
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. |
@t-hamano you're absolutely right... Thank you for catching that! 👍 |
Since there may be cases where multiple forms are inserted on a page, simply setting |
@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 |
@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
|
What about using the This code is an example. |
The simplest change I can think of, is to allow values like |
Huh... Good call! Thank you @t-hamano! Pushed a commit implementing that filter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 } );
};
Makes sense... Done. |
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 |
Yeah also you can pretty much always circumvent it via a synced pattern |
Since the concerns for this PR were addressed, I went ahead and merged it. |
const DISALLOWED_PARENTS = [ 'core/form' ]; | ||
addFilter( | ||
'blockEditor.__unstableCanInsertBlockType', | ||
'removeTemplatePartsFromPostTemplates', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do template parts and post templates have to do with this? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, this namespace would need to be changed to something like removeFormFromInserter
😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, something like core/block-library/preventInsertingFormIntoAnotherForm
would fit better. I don't think we have ever established a good pattern for naming filters.
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?
parent
to usingancestor
inblock.json
files for input blockscore/columns
andcore/group
blocks in the allowed blocks for formsTesting Instructions
Testing Instructions for Keyboard
Not applicable