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

Improve Documentation #2845 - lexical/utils #4047

Merged
merged 5 commits into from
Mar 10, 2023
Merged

Improve Documentation #2845 - lexical/utils #4047

merged 5 commits into from
Mar 10, 2023

Conversation

HarrySIV
Copy link
Contributor

@HarrySIV HarrySIV commented Mar 8, 2023

With Acy's help I've tried to follow his example and add inline api documentation, though I might have gone a bit overboard. Please let me know if there's anything I can do to improve the PR.

@vercel
Copy link

vercel bot commented Mar 8, 2023

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

Name Status Preview Comments Updated
lexical ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 10, 2023 at 5:18PM (UTC)
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 10, 2023 at 5:18PM (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 Mar 8, 2023
@HarrySIV
Copy link
Contributor Author

HarrySIV commented Mar 8, 2023

I'm really unsure as to why the e2e tests are failing. I wonder if using typedoc inline comments inside another one caused an issue, but I don't really know.

@acywatson
Copy link
Contributor

e2es are just being flaky

the vercel deployment is failing because of this:



Expand 62 Lines
--
17:33:57.095 | SyntaxError: /vercel/path0/packages/lexical-website/docs/api/modules/lexical_utils.md: Expected corresponding JSX closing tag for <div>. (538:70)
17:33:57.095 | 536 \| eg. element = `}<div class="element-inner active small">{`
17:33:57.096 | 537 \| removeClassNamesFromElement(element, `}{`['active small', true, null]`}{`);

@@ -226,22 +285,45 @@ export function $findMatchingParent(

type Func = () => void;

/**
* Executes each function passed into the mergeRegister function argument.
Copy link
Contributor

Choose a reason for hiding this comment

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

Almost. What this actually does is returns a function that will execute all of the functions passed to mergeRegister when called. The purpose is to make it easier to register a bunch of lexical listeners and then tear them down with a single function call. For example, in a React useEffect.

This one could potentially benefit from an @example annotation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This took me a minute to wrap my head around, but I think I understand how it works especially with useEffect. I hope my explanation is clear and helpful.

export function mergeRegister(...func: Array<Func>): () => void {
return () => {
func.forEach((f) => f());
};
}

/**
*
Copy link
Contributor

Choose a reason for hiding this comment

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

This attempts to resolve nested element nodes of the same type into a single node of that type. used mostly with marks/commenting

/**
* @param node - node to be tested against the targetNode instance
* @returns true if there is a match, false otherwise
*/
const $isTargetNode = (node: LexicalNode | null | undefined): node is N => {
Copy link
Contributor

Choose a reason for hiding this comment

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

These next ones aren't exported, so let's remove the comments

const $isTargetNode = (node: LexicalNode | null | undefined): node is N => {
return node instanceof targetNode;
};

/**
* Looks for an ancestor of a node of type targetNode. Ensures there are not any children of the node of the same targetNode instance
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

@@ -270,6 +352,10 @@ export function registerNestedElementResolver<N extends ElementNode>(
return null;
};

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

@@ -307,6 +393,12 @@ export function registerNestedElementResolver<N extends ElementNode>(
return editor.registerNodeTransform(targetNode, elementNodeTransform);
}

/**
* Clones the editor and marks it as dirty to be reconciled. If there was a selection,
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder how this is different from setEditorState... We can leave as-is for now and polish it up later.

@HarrySIV HarrySIV requested review from acywatson and removed request for trueadm, thegreatercurve and tylerjbainbridge March 9, 2023 19:16
@acywatson
Copy link
Contributor

Hmm - looks like a prettier error in there somewhere. Can you run prettier locally to see if you can figure out what the issue is?

@HarrySIV
Copy link
Contributor Author

Looked like some extra whitespace at the end of some sentences. Sorry...

@acywatson acywatson merged commit 60e17dc into facebook:main Mar 10, 2023
@HarrySIV HarrySIV deleted the inline-api-documentation-lexical/utils branch March 11, 2023 00:35
@zurfyx zurfyx mentioned this pull request Mar 24, 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.

None yet

3 participants