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

Move default language setting to Tokenizer #3368

Merged

Conversation

saint1991
Copy link
Contributor

@saint1991 saint1991 commented Nov 16, 2022

1. Add a preserve feature to CodeNode

In the original implementation, CodeNode is removed when the backward deletion occurs at the beginning of the Node.
In some use cases, implementing a code editor, such behavior is not needed.
The "Preserve" setting is for making it controllable.

2. Move the default language setting to Tokenizer

The former PR #3243 enabled to use arbitrary Tokenizer other than Prism and 'javascript' is not always appropriate to the default so make it a Tokenizer setting.

This PR contains 2 changes but I'm happy to divide them if required.

@vercel
Copy link

vercel bot commented Nov 16, 2022

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

Name Status Preview Updated
lexical ✅ Ready (Inspect) Visit Preview Dec 1, 2022 at 10:49AM (UTC)
lexical-playground ✅ Ready (Inspect) Visit Preview Dec 1, 2022 at 10:49AM (UTC)

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 16, 2022
@saint1991 saint1991 changed the title Tweak code highlight Add preserve feature to CodeNode Nov 18, 2022
Copy link
Contributor

@thegreatercurve thegreatercurve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saint1991 The default language changes seems fine, but I think we'd want to leave out the preserve flag. Users can just implement this themselves by extending the CodeNode and overwriting the collapseAtStart method.

@saint1991
Copy link
Contributor Author

saint1991 commented Nov 28, 2022

@thegreatercurve

Thanks for your kind review!
Thanks to #3367, the code something like below works as I expected.

Thank you so much for the great work!
I've removed commit 1. which is no longer needed as you mentioned.

export class CodeEditorNode extends CodeNode {
  static getType(): string {
    return 'code-editor';
  }

  collapseAtStart(): true {
    return true;
  }
}

export const $createCodeEditorNode = (language?: string | null | undefined): CodeEditorNode => {
  return new CodeEditorNode(language);
}

const editor = createEditor({
  nodes: [
    CodeEditorNode,
    CodeNode: {
      replace: CodeNode, 
      with: (node: CodeNode) => $createCodeEditorNode(node.getLanguage())
    },
    CodeHighlightNode,
  ]
})

@@ -33,7 +33,7 @@ declare export class CodeNode extends ElementNode {
setLanguage(language: string): void;
getLanguage(): string | void;
}
declare export function $createCodeNode(language?: string): CodeNode;
declare export function $createCodeNode(language?: string | null): CodeNode;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should just be enough:

Suggested change
declare export function $createCodeNode(language?: string | null): CodeNode;
declare export function $createCodeNode(language: ?string): CodeNode;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thegreatercurve
Thanks. Fixed it.

@fantactuka
Copy link
Contributor

I'm curious if it's better to have default language as a param to registerCodeHighlighting and CodeHighlightingPlugin. Then you can have single tokenizer but custom default language based on specific surface needs

@saint1991
Copy link
Contributor Author

I think it's also OK, but I put the setting here because the supported languages depend on Tokenizer's implementation. The setting 'javascript' is one of the proper choices for PrismTokenizer because Prism supports it.

In terms of the use of the library, the difference is that users should specify the default language even when they use PrismTokenizer by default.

@thegreatercurve thegreatercurve merged commit 9fbc564 into facebook:main Dec 8, 2022
@trueadm trueadm mentioned this pull request Dec 9, 2022
abelsj60 pushed a commit to abelsj60/lexical that referenced this pull request Jan 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants