-
-
Notifications
You must be signed in to change notification settings - Fork 605
refactor: move all blocks to use the custom blocks API #1904
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
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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.
Looks good! Just kind of glanced over everything for now with mostly questions
@@ -111,6 +117,37 @@ export function getParseRules( | |||
|
|||
return props; | |||
}, | |||
getContent: |
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.
Do you think it would be possible to return the content element in the parse
function, instead of having a separate parseContent
? Because I know they're split in the PM API, but I think it would be more intuitive to merge them into 1 function for the custom block API.
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.
Yea, I thought about this too. Right now, it is pretty easy to do a mapping of HTML -> BlockNote node. But what isn't clear is how to do that mapping for parsing of inner content. We've been going through Prosemirror DOMParser & doing this low-level. I tried a couple of different approaches to do it more high-level & couldn't figure out anything satisfactory.
So, I went with the approach of a "good default", (merge paragraphs to preserve content within the parsed element), and left the rest as an exercise for later. I think what will be useful in this is when we separate out the HTML parsing & exporting from the editor instance, then maybe it will be easier for consumers to parse HTML into the shape that they need 🤷. I marked it as an "advanced" API so that it is clear we are not happy with it. Right now the only time that any of the default blocks need to use parseContent is the list item types, because we added better handling of HTML -> list items.
); | ||
|
||
if (existingDecorations.length === 0) { | ||
// Create a widget decoration to display the index |
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.
Comment needs updating
@@ -32,8 +32,8 @@ export function checkDefaultBlockTypeInSchema< | |||
S | |||
> { | |||
return ( | |||
blockType in editor.schema.blockSchema && | |||
editor.schema.blockSchema[blockType] === defaultBlockSchema[blockType] | |||
blockType in editor.schema.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.
This is just a temp change while the updated blocks are not yet in the default schema right?
name: key, | ||
priority: ext.priority, | ||
addProseMirrorPlugins: () => ext.plugins, | ||
// TODO maybe collect all input rules from all extensions into one plugin |
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 it's more intuitive to keep logic for a block/extension within the same extension
@@ -105,3 +105,6 @@ export class BlockNoteSchema< | |||
this.styleSchema = getStyleSchemaFromSpecs(this.styleSpecs) as any; | |||
} | |||
} | |||
|
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: Remove?
@@ -21,6 +21,8 @@ export type BlockNoteDOMAttributes = Partial<{ | |||
[DOMElement in BlockNoteDOMElement]: Record<string, string>; | |||
}>; | |||
|
|||
// TODO we should remove FileBlockConfig, and only use BlockConfig | |||
// Ideally something like this would be represented via `groups: ["file"]` or similar |
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.
Could we add a file
field to the new meta
property in the config?
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 sure on this. It feels wrong to me that files are somehow different. Like why is there a file field & not a listItem field? I am not in favor of adding more properties than needed, since it adds complexity to each custom block that people can add.
I don't fully understand why this is even required at this point
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. posted some first thoughts / questions!
return true; | ||
}), | ||
}, | ||
inputRules: [ |
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.
cool that there's a BN-style API for this. Can we do the same for keyboardshortcuts you think?
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 thought about what that would look like for keyboard shortcuts, but the problem is that keyboard shortcuts can sort of just do anything. Some usecases I thought of were:
- bringing up some menu items
- triggering some action (e.g. stop AI generation)
- inserting a new element
- modifying an existing block
These sorts of cases are too disparate to really "unify" into a sensible API. Fundamental tension between composability & centralization.
* Advanced parsing function that controls how content within the block is parsed. | ||
* This is not recommended to use, and is only useful for advanced use cases. | ||
*/ | ||
parseContent?: (options: { el: HTMLElement; schema: Schema }) => Fragment; |
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.
would be nice to do this in a way that we don't expose Prosemirror APIs (i.e.: not expose Fragment) - not sure this is possible
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.
Yea I couldn't find a great way to do this. Especially given that the only case that really needs this are list items
* The BlockNote editor instance | ||
*/ | ||
editor: BlockNoteEditor<Record<TName, BlockConfig<TName, TProps>>>, | ||
) => { |
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.
These functions (dom / contentDOM / ignoreMutation) are very prosemirror specific. For now out of scope (as they're also in the existing API and this PR is about moving blocks over), but don't think this is ideal
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.
Yep, agreed, but not looking to rock that boat right now
}, | ||
}), | ||
() => [ | ||
createBlockNoteExtension({ |
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.
What was the reasoning again to define keyboardshortcuts / inputrules in a separate argument + createBlockNoteExtension
?
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 introduces keyboard shortcuts & inputrules to the extension API because we didn't have a way of doing them before.
I considered exposing them to both APIs (e.g. a custom block has an input rule API & so does the extension API), but that only added complexity.
Allowing extensions to be included to the block API is useful in that some things may require an extension to function (e.g. code block syntax highlighting, numbered list decorations).
@@ -41,8 +41,8 @@ export const alertBlock = createReactBlockSpec( | |||
textAlignment: defaultProps.textAlignment, | |||
textColor: defaultProps.textColor, | |||
type: { | |||
default: "warning", | |||
values: ["warning", "error", "info", "success"], | |||
default: "warning" 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.
is the as const
necessary now where before it wasn't? it's not ideal imo (the as const
syntax is probably not understood by basic TS devs, so if we can avoid it that'd be better ofc)
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 was necessary before too, this example just was not typed properly from the get go. So it isn't worse, it just isn't any better. TS infers an object within an object like this by default. I think we can just as const
the whole object though so it can be simpler.
typeof pageBreakSchema.blockSchema & | ||
typeof multiColumnSchema.blockSchema, | ||
DefaultBlockSchema & { | ||
pageBreak: ReturnType<typeof createPageBreakBlockConfig>; |
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.
minor: would be nice to have this consistent between pagebreak / multicolumn
meta: { | ||
isolating: false, | ||
}, | ||
}) 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.
fyi, as we now use createBlockSpec
we use custom NodeViews for all blocks, right?
I asked Marijn a long time ago if this can have negative consequences; https://discuss.prosemirror.net/t/pros-cons-of-custom-nodeviews-vs-regular-nodespec/5999
Do we need specific update logic? what's the default update logic for non-nodeview nodes?
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 spent a good amount of time looking into this.
This is the default implementation of a NodeView's update method, It checks that the node type is the same as the currently rendered one - and only if it isn't, will it recreate the node view.
I think we should hold off on this until we have an actual use case, before trying to come up with a node view that does anything more than this. You would need this method for a nodeview which is carrying state that you may want to preserve (if the node is updating often). In the default blocks, this would really only be the code block extension to save a re-instantiation of the highlighter in specific situations. But, if we don't update when we should have, the nodeview can become out of sync with the document state.
So, I think we can safely hold off on this for now & come back to it when it is a problem
render() { | ||
const pageBreak = document.createElement("div"); | ||
|
||
pageBreak.className = "bn-page-break"; |
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.
do we need both bn-page-break and data-page-break?
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'll leave that decision to @matthewlipski
}, | ||
[ | ||
createBlockNoteExtension({ | ||
key: "quote-block-shortcuts", |
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 think about a higher level (not transaction) specific API for the keyboard shortcuts. I see I commented this earlier, but let's discuss
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 don't know that there is one. Keyboard shortcuts can do pretty much anything. There could be an easier API for doing things like adding a new block of a certain type or updating a block to a certain type. But I'd argue that is useful outside of keyboard shortcuts so it is better to leave them be top-level things (e.g. editor.addBlock({ type: string; props: Record<string, unknown> })
)
If you have something specific you'd like to see, feel free to mention it!
dom: quote, | ||
contentDOM: quote, | ||
}; | ||
}, |
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'd be interested to see what we can come up with if we think about this API further. Things I think are within reach:
- decouple this further from block specs / schema
- make it easy to change / disable shortcuts / input rules
Related to this comment: https://blocknotejs.slack.com/archives/C01G3C47338/p1756719142481359?thread_ts=1756716444.502609&cid=C01G3C47338
maybe brainstorm together this week at office? good to research how the (de)coupling (between editor behavior vs schemas vs rendering) works in other editors?
Decoration.node(pos + 1, pos + node.nodeSize - 1, { | ||
"data-index": index.toString(), | ||
// TODO figure out start? is this needed? | ||
// "data-start": hasStart ? index.toString() : 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.
TODO (just commenting here to make sure we don't miss it)
defaultStyleSpecs, | ||
} from "./defaultBlocks.js"; | ||
|
||
export class BlockNoteSchema< |
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 do we have both BlockNoteSchema and CustomBlockNoteSchema?
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.
BlockNoteSchema
is a thin wrapper over CustomBlockNoteSchema
. CustomBlockNoteSchema
is the actual implementation & does not set any of the default blocks & requires the schema to be fully specified.
BlockNoteSchema
will pre-load all of the default blocks too, this separation is to allow BlockNoteSchema
to be tree-shaken off if it is not needed by someone. They can just use CustomBlockNoteSchema
and provide everything they will actually include in their editor.
@@ -9,6 +9,7 @@ export type PropSpec<PType extends boolean | number | string> = | |||
| { | |||
// We infer the type of the prop from the default value | |||
default: PType; | |||
type?: "string" | "number" | "boolean"; |
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.
sure this is needed? previously this was inferred from the default I think
), | ||
// disable default props on paragraph (such as textalignment and colors) | ||
{}, | ||
); | ||
|
||
// remove textColor, backgroundColor from styleSpecs | ||
const { textColor, backgroundColor, ...styleSpecs } = defaultStyleSpecs; | ||
|
||
// the schema to use for comments | ||
export const schema = BlockNoteSchema.create({ |
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?
className: "bn-react-node-view-renderer", | ||
}, | ||
)(this.props!) as ReturnType<BlockImplementation["render"]>; | ||
} else { |
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.
when does this get called? wouldn't this be covered by toExternalHTML or am I missing a scenario?
|
Might want to address in the future: #1919 (comment)
TODO:
sub package?not in scope for now but would be cool in the future (and should be built towards)blockImplementation.render
is not a function