feat: Add better type definitions for block props #52
Conversation
|
@justlevine @ayushnirwal |
🦋 Changeset detectedLatest commit: 83f223a The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
packages/types/src/blocks/base.ts
Outdated
| } | ||
|
|
||
| export type BlockProps< | ||
| T extends Record< string, unknown > = Record< string, unknown >, |
There was a problem hiding this comment.
I think T should extend BlockData
There was a problem hiding this comment.
A Few minor changes required. Love to see the scope minimized back to the original scope of the issue ( Type Defs for Props and Attributes of Block Renderes ). Other types related to Block Manager class, Block overloading etc., should be handled later.
PS: update the PR description too. The issue link is broken.
| const customBlockDefinition = | ||
| BlockManager.blockDefinitions[ node.type ]; | ||
| const defaultBlockDefinition = defaultBlockDefinitions[ node.type ]; | ||
|
|
||
| if ( customBlockDefinition === null ) { | ||
| // If explicitly set to null in custom definitions, use default renderer | ||
| node.renderer = BlockManager.blockDefinitions.default || Default; | ||
| } else if ( customBlockDefinition ) { | ||
| // If custom definition exists, use it | ||
| node.renderer = customBlockDefinition; | ||
| } else if ( defaultBlockDefinition ) { | ||
| // If no custom definition but default definition exists, use default | ||
| node.renderer = defaultBlockDefinition; |
There was a problem hiding this comment.
Is this refactoring required now that we decided to minimize the scope of this PR to just add Type definitions and helper for BlockProps and BlockAttributes?
There was a problem hiding this comment.
It doesn't hurt to leave this here, since this is the desired behaviour anyways.
| // Removing renderer and children from props. Renderer should not be passed to the component. | ||
| delete props.renderer; | ||
| delete props.children; | ||
|
|
There was a problem hiding this comment.
This used to delete the keys too. Revert this change.
There was a problem hiding this comment.
I had to use Record<any, any> for the props currently. We should fix the renderer and prop types in a separate PR.
packages/query/package.json
Outdated
| @@ -45,6 +45,7 @@ | |||
| "peerDependencies": { | |||
| "react": "^19.0.0", | |||
| "@apollo/client": "^3.11.4", | |||
| "graphql": "^16.9.0" | |||
| "graphql": "^16.9.0", | |||
| "@snapwp/types": "*" | |||
There was a problem hiding this comment.
I may be wrong but the types package should be a DevDependencies. The types should be in the build artifacts. Please test that theory out and move to DevDependencies if Im correct
@SH4LIN Opinions?
There was a problem hiding this comment.
This should be in devDependencies. The type used in the build artifact will be referenced by other TypeScript files. Since all TypeScript files are compiled during development and not executed at runtime, it should be a devDependency.
justlevine
requested review from
SH4LIN and
ayushnirwal
and removed request for
ayushnirwal
What
This PR adds type definitions for block prop types.
Why
The exported block definitions can be used while overriding block definitions.
Related Issue(s):
https://github.com/rtCamp/snapwp/issues/53
How
The newly created prop types are exported from the component file. For now, the types are copied from the graphql generated types.
Testing Instructions
Screenshots
Additional Info
Checklist