-
Notifications
You must be signed in to change notification settings - Fork 30.5k
@wordpress/blocks: allow Partial BlockConfiguration as 2nd arg when 1st arg is metadata. #55960
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
@wordpress/blocks: allow Partial BlockConfiguration as 2nd arg when 1st arg is metadata. #55960
Conversation
…st arg is metadata.
|
@ZebulanStanphill Thank you for submitting this PR! I see this is your first time submitting to DefinitelyTyped 👋 — I'm the local bot who will help you through the process of getting things through. This is a live comment which I will keep updated. 1 package in this PRCode ReviewsBecause you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it. You can test the changes of this PR in the Playground. Status
All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes. Diagnostic Information: What the bot saw about this PR{
"type": "info",
"now": "-",
"pr_number": 55960,
"author": "ZebulanStanphill",
"headCommitOid": "977f9b88e0bcab181559dfd50967a740331db679",
"lastPushDate": "2021-09-23T21:45:23.000Z",
"lastActivityDate": "2021-09-23T22:54:46.000Z",
"mergeOfferDate": "2021-09-23T22:53:39.000Z",
"mergeRequestDate": "2021-09-23T22:54:46.000Z",
"mergeRequestUser": "ZebulanStanphill",
"hasMergeConflict": false,
"isFirstContribution": true,
"tooManyFiles": false,
"popularityLevel": "Well-liked by everyone",
"pkgInfo": [
{
"name": "wordpress__blocks",
"kind": "edit",
"files": [
{
"path": "types/wordpress__blocks/api/registration.d.ts",
"kind": "definition"
},
{
"path": "types/wordpress__blocks/wordpress__blocks-tests.tsx",
"kind": "test"
}
],
"owners": [
"dsifford",
"sirreal",
"dmsnell"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Well-liked by everyone"
}
],
"reviews": [
{
"type": "approved",
"reviewer": "dmsnell",
"date": "2021-09-23T22:30:56.000Z",
"isMaintainer": false
}
],
"mainBotCommentID": 926204565,
"ciResult": "pass"
} |
|
🔔 @dsifford @sirreal @dmsnell — please review this PR in the next few days. Be sure to explicitly select |
dmsnell
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.
Thanks for the fast fix @ZebulanStanphill - looks like a winner to me
| export function registerBlockType<TAttributes extends Record<string, any> = {}>( | ||
| metadata: BlockConfiguration<TAttributes>, | ||
| settings?: BlockConfiguration<TAttributes>, | ||
| settings?: Partial<BlockConfiguration<TAttributes>>, |
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.
I think we could find a way to say "you have to supply the missing metadata" here but no need in this PR. maybe a dog-ear for the future.
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.
I'm not entirely sure, but I think a block.json is expected to contain all the required properties anyway, so I don't think that would be necessary.
|
@ZebulanStanphill: Everything looks good here. I am ready to merge this PR (at 977f9b8) on your behalf whenever you think it's ready. If you'd like that to happen, please post a comment saying:
and I'll merge this PR almost instantly. Thanks for helping out! ❤️ |
|
Ready to merge |
Description
Follow-up to #55245.
The following should work (as seen in the official documentation), but currently, the 2nd arg is considered invalid:
The problem is that the 2nd arg, while optional, still requires all the same props if present, whereas with the new signature, it shouldn't require any.
This PR fixes that issue by adding a
Partialaround the 2nd arg in the signature that accepts a metadata object as the first argument.Checklists
Please fill in this template.
npm test <package to test>.Select one of these and delete the others:
If changing an existing definition: