-
-
Notifications
You must be signed in to change notification settings - Fork 491
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
simple latex block #818
simple latex block #818
Conversation
# Conflicts: # packages/core/src/api/blockManipulation/__snapshots__/blockManipulation.test.ts.snap # packages/core/src/api/blockManipulation/blockManipulation.test.ts # packages/core/src/api/nodeConversions/__snapshots__/nodeConversions.test.ts.snap
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@jkcs 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 already looks pretty good, awesome!
I've left some comments inline, and have some additional questions:
- Why did you need to use
createInternalInlineContentSpec
instead of the regular content spec? - It would be very nice to have this keyboard accessible (compare with the behavior in Notion when using the keyboards to navigate).
- Should we create a separate package (e.g.:
packages/react-extension-equations
) for this so users can easily add it to their editor without having to copy all the code? I think it should be an extension because not all users might want to pull in the katex dependency
It's great to see your contributions; it seems you're getting up to speed nicely 💪 did you find it ok to get started and / or have any feedback about the codebase?
import { ChangeEvent, useEffect, useState, useRef } from "react"; | ||
import "./styles.css"; | ||
|
||
function loadKaTex(callback: () => void) { |
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 prefer to just have this as a dependency of the package, that makes the code a lot cleaner imo
@@ -139,7 +140,11 @@ export function getProjectFiles(project: Project): Files { | |||
*/ | |||
export function getExampleProjects(): Project[] { | |||
const examples: Project[] = glob | |||
.globSync(path.join(dir, "../../../examples/**/*/.bnexample.json")) | |||
.globSync( |
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 suppose this is needed for windows?
Cleaner might be: path .join(dir, "..","..","..","examples","**",".*","..bnexample.json")
?
return nodeView; | ||
} | ||
|
||
const 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.
I don't think open
should be part of the blockschema. It's a state of the UI, not of the document.
For example, this will mean that the open state will be stored in databases or synced over collaboration, I don't think that's what we want
Getting started with the code was okay for me, having prior experience with Tiptap would have made it faster. It would be great if we could enhance the CONTRIBUTING.md file. Given this, I will create a new PR |
Ok!
Awesome. Yes, feel free to make a PR for the file with improvements you'd like to see!
Why do you need your manual NodeView? If there is anything missing in our higher level |
The implementation of keyboard accessibility is more challenging than I anticipated. I'll close this for now and will provide a new PR once I have it implemented |
Hi @jkcs! Any progress on this? I think you were doing great, would be awesome to be able to merge this at some point :) How did it go with the keyboard approach? |
The progress with keyboard accessibility is not going smoothly. I will continue working on it when I have more free time, which should be within a few week |
Hey @jkcs how is it going ? I would like to collaborate, recently worked on a similar issue (emoji picker) |
I saw it, well done. I'll restart this today, and if I need help, I'll definitely let you know |
close #741
/claim #741