Skip to content
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

Closed
wants to merge 14 commits into from
Closed

simple latex block #818

wants to merge 14 commits into from

Conversation

jkcs
Copy link
Contributor

@jkcs jkcs commented Jun 7, 2024

close #741
/claim #741

jkcs added 11 commits June 4, 2024 15:32
# 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
Copy link

vercel bot commented Jun 7, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
blocknote ✅ Ready (Inspect) Visit Preview Jun 8, 2024 3:32am
blocknote-website ✅ Ready (Inspect) Visit Preview Jun 8, 2024 3:32am

Copy link

vercel bot commented Jun 7, 2024

@jkcs is attempting to deploy a commit to the TypeCell Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Collaborator

@YousefED YousefED left a 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) {
Copy link
Collaborator

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(
Copy link
Collaborator

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 = {
Copy link
Collaborator

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

@jkcs
Copy link
Contributor Author

jkcs commented Jun 11, 2024

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?

  1. The reason for using createInternalInlineContentSpec is that it allows customization of addNodeView. (Not sure if createInlineContentSpec would work)
  2. I will look into keyboard accessibility.
  3. You’re right, we should create a separate package.

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

@YousefED
Copy link
Collaborator

YousefED commented Jun 11, 2024

Ok!

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.

Awesome. Yes, feel free to make a PR for the file with improvements you'd like to see!

The reason for using createInternalInlineContentSpec is that it allows customization of addNodeView. (Not sure if createInlineContentSpec would work)

Why do you need your manual NodeView? If there is anything missing in our higher level createReactInlineContentSpec, it'd be great to know so we can consider implementing it there. This way we would improve our API surface + not have the Latex block dive into "internals"

@jkcs
Copy link
Contributor Author

jkcs commented Jun 14, 2024

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

@jkcs jkcs closed this Jun 14, 2024
@YousefED
Copy link
Collaborator

YousefED commented Jul 9, 2024

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?

@jkcs
Copy link
Contributor Author

jkcs commented Jul 10, 2024

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

@hunxjunedo
Copy link
Contributor

Hey @jkcs how is it going ? I would like to collaborate, recently worked on a similar issue (emoji picker)

@jkcs
Copy link
Contributor Author

jkcs commented Jul 19, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom Block for Katex
3 participants