Skip to content
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

Jetpack Blocks: Add the jetpack contact form blocks to the preset #29150

Closed
wants to merge 2 commits into from

Conversation

enejb
Copy link
Member

@enejb enejb commented Dec 5, 2018

Changes proposed in this Pull Request

  • Add the contact form presets previously missing.

Testing instructions

  • Build this PR in Jetpack and .com does things work as expected?
  • Are you able to load create the contact form as expected?

We missed this in the previous release and allowed all the blocks to be loaded as they were.

@enejb enejb added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 5, 2018
@matticbot
Copy link
Contributor

@enejb enejb self-assigned this Dec 5, 2018
@enejb enejb requested review from sirreal, roccotripaldi and a team December 5, 2018 17:40
@simison
Copy link
Member

simison commented Dec 5, 2018

How can I observe the brokeness without this change? :-)

@ockham
Copy link
Contributor

ockham commented Dec 5, 2018

Wondering the same as @simison 🙂 Personally, I guess I'd rather avoid having to include child blocks explicitly in a lot of places...

Also noting that this PR might impact #29134...

@enejb
Copy link
Member Author

enejb commented Dec 5, 2018

@simison I want to make the following change to jetpack Automattic/jetpack#10844

Without it this PR will not work as expected.

@ockham
Copy link
Contributor

ockham commented Dec 5, 2018

I"m all for de-duplicating information, but TBH, adding all those child blocks to index.json clutters it up quite a bit. I'd love if we could just rely on the parent block (i.e. Contact Form) to pull in its children as needed...

@jeherve jeherve added [Goal] Gutenberg Working towards full integration with Gutenberg [Block] Contact Form labels Dec 6, 2018
@tyxla
Copy link
Member

tyxla commented Dec 7, 2018

I agree with @ockham and @simison, and I'm wonderijng - what's the value of specifically declaring all the child blocks in index.json?

@gititon
Copy link
Contributor

gititon commented Dec 10, 2018

I agree that if the presence of the parent block necessarily means that its child blocks are also included, index.json is much tidier when only the parent block is listed.

@enejb enejb force-pushed the update/jetpack-preset-block.json branch from 04e9416 to a47dcad Compare December 11, 2018 22:23
@enejb
Copy link
Member Author

enejb commented Dec 12, 2018

@tyxla, @gititon, @simison and @ockham.
There is no such thing as child blocks. As far as I know. The reason why I think we should have the complete list of blocks available in index.js is because it simplifies how we reason about things. If the item is in this list then the block is something that is available. There is no secondary list. That also influences things. At least we can make that happen for the beta blocks an wpcom proxied. Right now how things work is a bit of a mess.

The only reason why things work now is because we merge the current default blocks with the list in the index. The list of default blocks is hard coded in the Jetpack Gutenberg class.

Here is the PR that cleans it up on jetpack. Automattic/jetpack#10844

@tyxla
Copy link
Member

tyxla commented Dec 12, 2018

There is no such thing as child blocks.

How come? Gutenberg has the concept of nested blocks AKA inner blocks, AKA child blocks - blocks that can only be inserted as nested blocks inside a limited set of parent blocks. Regardless of how we call them, it is a concept that exists in Gutenberg core, and we use it as well.

I don't see how that is a mess. I think that any block should be able to register child blocks, and block availability wouldn't have to know about them. That would change only in the event where we need to hide certain nested blocks for reasons (pain plans, etc.), and I don't see such a use case yet.

Personally, I think treating the nested blocks as regular blocks makes things more messy.

@enejb
Copy link
Member Author

enejb commented Dec 12, 2018

How come? Gutenberg has the concept of nested blocks AKA inner blocks, AKA child blocks - blocks that can only be inserted as nested blocks inside a limited set of parent blocks. Regardless of how we call them, it is a concept that exists in Gutenberg core, and we use it as well.

Yes there are such concepts as inner blocks but the inner blocks could have more then one parent. In gutenberg inner blocks define who the parent is and not the other way around. In someways it would make more sense to not include the form block because we could conditionally include it because a inner block knows about it. (I am not arguing for this just want to clarify things)

InnerBlocks still need to be register individually. If we only had to register the parent block to get the inner blocks I think what we currently have would make sense.

We are trying to separate the block definitions from when we register them. So having some block in the index and not the other ones doesn't make sense to me. The list is not complete.

The only reason why things work now is because of a bug. That I am trying to get rid of on the jetpack side. See Automattic/jetpack#10844

For example if I am working on an inner block that I want to beta tests before I release. How do we do that right now?

@enejb enejb force-pushed the update/jetpack-preset-block.json branch from a47dcad to 84ed8fe Compare December 12, 2018 19:38
@gititon
Copy link
Contributor

gititon commented Dec 12, 2018

I've come around and agree with Enej that each block should be listed explicitly. This would ensure that there is a single source of truth for the blocks that are included, as opposed to some blocks being loaded by index.json and others being included as part of the Jetpack default blocks. As Enej explained to me, this would also facilitate beta testing inner blocks before they are added to production.

That said, I propose we indent the "child blocks" beneath the Contact Form block. While the whitespace will be inert, it will clearly distinguish these blocks as being child blocks of the Contact Form block as opposed to being "top level" blocks. This would make block inclusions explicit while at the same time keeping the list looking organized. E.g.,

{
  "production": [
    "contact-form",
        "field-text",
        "field-name",
        "field-email",
        "field-url",
        "field-date",
        "field-telephone",
        "field-textarea",
        "field-checkbox",
        "field-checkbox-multiple",
        "field-radio",
        "field-select",
    "map",
    "markdown",
    "publicize",
    "simple-payments"
  ],
  "beta": [
    "related-posts",
    "tiled-gallery",
    "vr"
  ]
}

@tyxla
Copy link
Member

tyxla commented Dec 13, 2018

I've come around and agree with Enej that each block should be listed explicitly. This would ensure that there is a single source of truth for the blocks that are included, as opposed to some blocks being loaded by index.json and others being included as part of the Jetpack default blocks.

I'd prefer to go YAGNI with this. Do we have a real use case where this would be useful?

As Enej explained to me, this would also facilitate beta testing inner blocks before they are added to production.

This is likely the only good reason I see that could justify the added complexity. I wonder if we could find a better way...

That said, I propose we indent the "child blocks" beneath the Contact Form block.

What if a block can be a child block to multiple parent blocks?

@sirreal
Copy link
Member

sirreal commented Jan 7, 2019

I'm closing because I believe this has been superseded by other work.

If that's not the case, please reopen.

@sirreal sirreal closed this Jan 7, 2019
@sirreal sirreal deleted the update/jetpack-preset-block.json branch January 7, 2019 12:43
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Contact Form [Goal] Gutenberg Working towards full integration with Gutenberg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants