-
-
Notifications
You must be signed in to change notification settings - Fork 456
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
feat: Code block #1177
base: main
Are you sure you want to change the base?
feat: Code block #1177
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@areknawo is attempting to deploy a commit to the TypeCell Team on Vercel. A member of the Team first needs to authorize it. |
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 looks great!
I think the only things remaining would be to add this block to the nodeconversions / htmlconversions unit tests (we can also do this) and to the default schema showcase (https://blocknote-git-fork-areknawo-main-typecell.vercel.app/basic/all-blocks)
I also noticed that we don't correctly set the language when it's parsed from markdown (you can try this here: https://blocknote-git-fork-areknawo-main-typecell.vercel.app/interoperability/converting-blocks-from-md).
One other thing I'm in doubt of is which languages we should show in the default dropdown. Maybe it makes more sense to limit options to some popular languages by default, what do you think?
return { | ||
indentLineWithTab: true, | ||
supportedLanguages: [ | ||
{ |
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.
tbd: maybe we should use plain text or javascript as default
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 also wasn't sure about this. Usually, in HTML/MD, code blocks without language simply leave it out, so I made an empty string the default and assigned a Code label to it. I think the empty string makes sense, but label can be changed
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 should have a text
language (https://shiki.style/languages#plain-text) for plain text.
Then we can still decide to upon creation initialize with plain text vs javascript. Maybe let's do javascript for now because otherwise people don't really notice the syntax highlighting
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.
Ok, I think I can add defaultLanguage
option with text
as the default.
For text
, txt
, plain
values, I understand renderHTML
should skip them, not to add class="language-text"
to the HTML?
!$from.parent.textContent && | ||
isTextSelection(selection) | ||
) { | ||
const from = $from.pos - $from.parentOffset - 2; |
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 you explain what's happening here? Or add a comment in the code?
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 mean a specific line or the keyboard shortcut as a whole?
As for the shortcut - this one removes an empty code block on DELETE
from the inside (common behavior across different WYSIWYG editors that didn't work out-of-the-box in BlockNote). I can add 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.
The specific line. My reasoning; when I read this code with a manual position fix in the review, I figured there's probably something interesting going on - so I was triggered to ask about this. From your reply that seems to be the case indeed, so let's document it with a comment and turn this into shared knowledge.
TLDR: if there's a "workaround" needed for something, let's try to document that as much as possible in the code for others or your "future self"
@@ -3,7 +3,7 @@ | |||
"private": true, | |||
"version": "0.17.1", | |||
"scripts": { | |||
"dev": "vite", | |||
"dev": "vite --host", |
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.
does this do anything when not passing a host?
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 exposes the dev server on the local network. I didn't mean to leave it, but it's helpful for testing on different devices
I see that you specifically modify rehype handler to use a
Wasn't sure about this, especially with Shiki having support for different language variants (e.g. |
}; | ||
|
||
export const codeBlockPropSchema = { | ||
...defaultProps, |
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.
@YousefED Should the code block support default props (e.g. textAlignment
, backgroundColor
, etc.). I've referenced it from other blocks, but I'm not sure if it makes sense here
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.
good spot, let's remove
Cool. newline, not sure, let's remove it (might have been TypeCell related)
What about this list: https://shiki.style/languages -> web bundle, but
What do you think? |
I think it's a good selection. It's always going to be a bit subjective, but it makes sense to shorten it a little bit by default |
This PR adds a code block with syntax highlighting (using prosemirror-highlight) to the core.