-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Content only block experiment: blocks provide the form #73479
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
Conversation
|
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. |
|
Size Change: +225 B (+0.01%) Total Size: 2.54 MB
ℹ️ View Unchanged
|
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.
Thanks for jumping in to collaborate here, much appreciated! Something I'm wondering: rather than an additional formKey attribute, I kind of wish we could re-use the isVisible prop for fields. Currently it only supports a function, but it'd be neat if we could set it to be a boolean. Then we could replace shownByDefault with an isVisible prop, rather than also needing to define the form.
What do you think? Would that be something helpful to expand on in the Fields API (supporting a boolean rather than just a function)? It might help simplify things a little 🤔
|
@andrewserong The form needs to be a standalone concept in the blocks. It matches the DataForm The field |
How so? It looks like the only usage right now is for the visibility of fields, and I'm not quite sold on the idea of blocks themselves defining the overall form, as the Content Only Controls are quite a particular kind of UI and so fairly opinionated. I guess what I'm trying to say is that I understood the sidebar UI itself to be the owner of what the overall form looks like, with blocks defining fields that could be present in that form. I've very much been informed by useful feedback from @youknowriad in this regard, so my ideas here are very much weakly held! My main concern with this PR is that it feels like adding an additional API to include a Before proceeding to add an additional block
Yes, I think that's what I was getting at in my earlier feedback. Is this an opportunity to expand the Fields API to allow boolean values for
The value to me is that we wouldn't need to define a form at the same time from within the block settings, it could be a prop on a field instead of needing to have a form object. The behaviour surrounding showing/hiding fields is still pretty experimental in the ContentOnlyControls, so I'm mostly trying to think about how we can rein in complexity... which is quite the challenge! 😄 All that said, this work is still behind an experiment, so I don't want to hold up progressing the work here! It's still possible for us to change things after the fact, so these discussions can continue in parallel. |
andrewserong
left a comment
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 testing well for me, and I'm not opposed to getting this in so that we can get the settings shape closer to the dataform and fields APIs shape.
Some ideas for follow-ups include:
- If no
settings.formkey is present, then create a basic form using the provided fields as-is - Look into what level of customisation we want blocks to have over the form. Is it only part of the form API, or the whole thing?
We can track and follow-up on these things in #73261.
Keen to hear what other folks think, too!
|
The important part to me is that we don't create a new API and instead leverage what we have (Field API, Form API). I'd be fine if we find another place to hold this info. At this stage, what I care about is that it is not part of the Field API the block exposes because that's not how it works elsewhere. |
|
I think block types having a "form" key to define how their inspector controls (attributes) combine feels like a good approach to me, but content-only "form" is a separate kind of form. So I wonder if ultimately we need two forms or potentially auto-remove non content only attributes/fields from the form object. I can see this "form" object being useful for instance for php-only registered blocks. |
Yep, this is what I think, the content only controls are like a filtered down version of the full controls. Filtered via the We're starting with content only controls, but I expect we eventually we can expand to cover all attributes or at least a broader subset. I expect there are a few blocks out there that could have their InspectorControls implementation entirely replaced with a generated DataForm if we can get the fields implementation right. |
|
Thanks for the discussion, folks!
Gotcha, so our first priority is to make sure that what we have for content only controls is consistent with the current API, and we can look into expanding that API further down the track (once we've tidied things up here). Sounds good! |
Follow-up to #73374
Part of: #73261
What?
This adds a new
settings.formKey(similar tosettings.fieldsKey) for the block type to provide a form object that declares what fields should be visible by default.Why?
This simplifies the logic and the code:
shownByDefaultkey. This also allows to leverage the DataForm API in next steps (using different layouts per field, etc.).Testing Instructions
Same as #73374