Fix table of contents for headings in tables and collapsible sections #5946
Conversation
KatsiarynaDzibrova
requested review from
zurfyx,
fantactuka,
acywatson,
Fetz and
ivailop7
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
the
CLA Signed
| ); | ||
| const headingNodes = dfsNodes.map( | ||
| (dfsNode) => dfsNode.node as HeadingNode, | ||
| ); |
There was a problem hiding this comment.
You can reduce from 2 loops to 1 loop using reduce or flatmap. Syntax should be something along those lines:
const headingNodes = $dfs().reduce((acc, {node}) => $isHeadingNode(node) ? [...acc, node] : acc, [])this also unpacks the dfs node, to use the object under it's 'node', thus why it's in curly braces, so you don't have to reference dfsNode.node everywhere. You can read on this here:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment
There was a problem hiding this comment.
Thank you. Added the change
| (acc, {node}) => ($isHeadingNode(node) ? [...acc, node] : acc), | ||
| [] as HeadingNode[], | ||
| ); | ||
| currentTableOfContents = $newTableOfContents(headingNodes); |
There was a problem hiding this comment.
I think we can do an extra performance optimization - we store the list, and we only replace it with the output from dfs on create, update for a headingNode, but on delete, we just delete from the map. What do you thing?
There was a problem hiding this comment.
Have we measured performance? Running a full $dfs every time that a HeadingNode is mutated might be fairly inefficient, that why the previous version uses mutated nodes. For reference, the reconciler doesn't look at all nodes either, instead it traverses through the dirty nodes, I would take the reconciler as an example for optimizing this plugin.
|
Made the following changes:
@zurfyx feel free to take a look |
packages/lexical-utils/src/index.ts
Outdated
| @@ -222,6 +222,29 @@ function $getDepth(node: LexicalNode): number { | |||
| return depth; | |||
| } | |||
|
|
|||
| export function $getPreviousNode( | |||
There was a problem hiding this comment.
It lacks a comment description, also confused by the naming getPreviousNode, yet in the first if statement, it gets the last child, which is not 'previous' to my understanding.
There was a problem hiding this comment.
Still unsure if this should be in utils or encapsulated in the TOC plugin file. Thoughts @zurfyx ?
Fixes following issues with table of contents:
Now on each heading update dfs is used to get all headings and create a new TOC from scratch. It might take more time, but the change shouldn't be noticeable
Before:
After:
