-
-
Notifications
You must be signed in to change notification settings - Fork 558
Custom blocks api changes #183
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 latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Great stuff!
- I think the way schemas (props / blocks) now work is a good improvement
- I'm not 100% convinced about the
NumberedListItemBlockContent
,TipTapNodeConfig
,TypesMatch
. It feels like this is quite some code (complexity) for a small (maybe I'm wrong here) gain in type-correctness. I think for these scenarios it's find to do a quick runtime check
Overall, great work. I've left some minor comments. I'll review the parse / renderhtml etc. when the first custom block tests have been added
console.log("test"); | ||
// apply defaults | ||
options = { | ||
const newOptions: Omit<typeof options, "defaultStyles" | "blockSchema"> & { |
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.
why first the omit
and then the addition with &?
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.
It's because options
is Partial
, so defaultStyles
and blockSchema
might be undefined. Since we're applying defaults to them this changes those 2 fields to always be defined.
|
||
if (validAttrs.find((prop) => prop.name === attr)) { | ||
if (attr in propSchema) { |
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 nice improvement now that we don't use an array anymore :)
: string; | ||
}; | ||
|
||
// Defines the config for a single block. Meant to be used as an argument to |
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.
nice docs!
}, | ||
} as const; | ||
|
||
const imageProps = { src: { default: "gfr" } } as const; |
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.
TODO, move to test
...ges/core/src/extensions/Blocks/nodes/BlockContent/HeadingBlockContent/HeadingBlockContent.ts
Outdated
Show resolved
Hide resolved
packages/react/src/FormattingToolbar/components/DefaultButtons/TextAlignButton.tsx
Outdated
Show resolved
Hide resolved
packages/react/src/FormattingToolbar/components/DefaultDropdowns/BlockTypeDropdown.tsx
Show resolved
Hide resolved
// Gets BlockNote editor instance | ||
const editor = this.options.editor!; | ||
// Gets position of the node | ||
const pos = typeof getPos === "function" ? getPos() : undefined; |
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.
throw error if pos is undefined, so you don't need pos!
below
|
||
// Render elements | ||
const rendered = blockConfig.render(props); | ||
const rendered = blockConfig.render(getDummyBlock, editor); |
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.
let's discuss this
superseded by #191 |
No description provided.