[lexical][lexical-react][lexical-table] Fix: Add horizontal scroll for tables #6713
Conversation
GermanJablo
requested review from
zurfyx,
fantactuka,
acywatson,
Fetz,
ivailop7,
Sahejkm and
potatowagon
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
the
CLA Signed
size-limit report 📦
|
| /** | ||
| * This node allows the nodes it wraps to have a width greater than that of the editor, | ||
| * encapsulating only its content in a horizontal scroll. You'll want to use it to wrap | ||
| * nodes that have a "width" property and are NOT decoratorNodes. See TableNode as an example. | ||
| */ | ||
| export class ScrollableNode extends ElementNode { |
There was a problem hiding this comment.
At first I wrapped TableNode and ImageNode, and so I put it in core. Then I realized that since it's a decoratorNode in ImageNode you can replicate the same behavior much easier without using this node. So I moved the node to lexical-table.
Wrapping another node with ScrollableNode should be easy if you want to do it in the future. At the moment TableNode is the only one that has a width property and it's not DecoratorNode.
There was a problem hiding this comment.
I think it would be tricky to use this class as-is with other element types because of the transform that requires the child to be a table
|
|
||
| createDOM(_config: EditorConfig, _editor: LexicalEditor): HTMLElement { | ||
| const dom = document.createElement('div'); | ||
| dom.classList.add('lexical-scrollable'); |
There was a problem hiding this comment.
Required for exportDOM and importDOM. I've included new tests to verify that it works correctly.
There was a problem hiding this comment.
I don't have time for QA right now or to audit the tests but I took a quick look at the code. Seems like a useful improvement! The downside is that it is a bit backwards incompatible due to the additional node and requirements for table use. Would maybe be a bit nicer if it was done in a way that could be backwards compatible. @ivailop7 should have a look
| /** | ||
| * This node allows the nodes it wraps to have a width greater than that of the editor, | ||
| * encapsulating only its content in a horizontal scroll. You'll want to use it to wrap | ||
| * nodes that have a "width" property and are NOT decoratorNodes. See TableNode as an example. | ||
| */ | ||
| export class ScrollableNode extends ElementNode { |
There was a problem hiding this comment.
I think it would be tricky to use this class as-is with other element types because of the transform that requires the child to be a table
| const selection = $getSelection(); | ||
| if ((node as ScrollableNode).isEmpty()) { | ||
| const root = $getRoot(); | ||
| if (root.__first === node.getKey() && root.getChildrenSize() === 1) { | ||
| root.append($createParagraphNode()); | ||
| node.remove(); | ||
| } else { | ||
| node.remove(); | ||
| } | ||
| return; | ||
| } | ||
| const firstChild: ElementNode = (node as ScrollableNode).getFirstChild()!; | ||
| if (firstChild && !$isTableNode(firstChild)) { | ||
| firstChild.remove(); | ||
| } | ||
| if ( | ||
| $isRangeSelection(selection) && | ||
| selection.anchor.key === node.getKey() | ||
| ) { | ||
| firstChild.getFirstDescendant()!.selectStart(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
It's not entirely clear what this is defending against but it doesn't look like it's doing it correctly since the last branch can execute when firstChild is not a table (might not have a first descendant) and is not connected to the root. I would put the child check before the empty check, remove all children except the first that passes the table check, and then if there's no table node found then remove the node (and fix up the root if necessary).
| if (direction === 'backward') { | ||
| siblingNode.selectEnd(); | ||
| } else { | ||
| const firstDescendant = siblingNode.getFirstDescendant()!; |
There was a problem hiding this comment.
it would be better to use a runtime invariant rather than a non-null assertion here, there isn't a runtime check that this is non-empty
Description
Closes #6480
Having tables overflow is a mistake, as their content can conflict with other elements of the interface:
Another incorrect behavior is the one that occurs in the Lexical Playground, where if the table is too large, the entire editor becomes larger. The horizontal scroll bar is not placed over the table, but over the entire content.
A super quick way to fix overflow is to add a max-width of 100% to the table.
The problem is, as this user noted, that this creates conflicts with the column width property.
This is what OneNote does and I've always hated it. Sometimes columns resize in a magical or hard-to-understand way. It's better to give the user the flexibility to adjust column widths to their liking and to have overflow if needed.
The correct solution to this is for tables to have an overflow scroll independent of the rest of the editor, similar to what Notion does:
The problem is that the
<table>tag doesn't support overflow: scroll. A div is needed to wrap it.This PR introduces a new
Scrollablenode that automatically wraps tables.BREAKING CHANGE:
TablePluginnow requires the editor to also register theScrollableNodenode. All tables are automatically wrapped in aScrollableNode.Test plan
Before
After