[lexical-playground][lexical-table] Bug Fix: Resizing table with merged cells #6200
Conversation
AlexanderReznik
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 📦
|
| @@ -178,6 +182,17 @@ function TableCellResizer({editor}: {editor: LexicalEditor}): JSX.Element { | |||
| throw new Error('Expected table row'); | |||
| } | |||
|
|
|||
| let height = tableRow.getHeight(); | |||
| if (!height) { | |||
There was a problem hiding this comment.
hi! really appreciate this fix! leaving some suggestions to make the code clearer
lets preferably avoid sketchy null checks. since height is number | undefined, if height = 0, it will be lumped under the undefined case.
hence would be great to have explicit if conditions, such as if (height === undefined || height === 0)
There was a problem hiding this comment.
Yes, makes sense!
Changed to more specific checks in 9fa9e0f
There was a problem hiding this comment.
thankyou for the updates!
| activeEditor: LexicalEditor, | ||
| ): number | undefined => { | ||
| const width = cell.getWidth(); | ||
| if (width) { |
There was a problem hiding this comment.
same here for sketchy null check, if width = 0 should this if block be skipped?
| } | ||
|
|
||
| const domCellNode = activeEditor.getElementByKey(cell.getKey()); | ||
| if (!domCellNode) { |
There was a problem hiding this comment.
| if (!domCellNode) { | |
| if (domCellNode == null) { |
| if (!$isTableCellNode(tableCell)) { | ||
| throw new Error('Expected table cell'); | ||
| const width = getCellNodeWidth(cell.cell, editor); | ||
| if (!width) { |
There was a problem hiding this comment.
| if (!width) { | |
| if (width === undefined) { |
|
Hi @AlexanderReznik this PR is actually failing the CI , the newly added e2e tests written are failing , example: https://github.com/facebook/lexical/actions/runs/9311543149/job/25630871373?pr=6216 Sorry we missed adding extended-tests label before merging this PR, could you help look into this ? Thank you! |
Yes, sure, I'll have a look! |
Changing how table resizing works
Changing the selection of cells that needs to be resized during resizing of a column
Current logic works nicely for the cases without merged cells, however it breaks after cells merging. I'm using
$computeTableMapSkipCellCheckto create a mapping of cells to their coordinates on screen on every resize. And I think it might be beneficial to cache the map in theTableNodeas it is useful for selection, resizing, navigation in a table with keyboard (that currently is also crashing with the merged cells). I'd appreciate the thoughts on whether it's possible to extend theTableNode.Changing the logic for calculating new width/height of cells
Assuming that the new height/width of the cell that is active during the resize should be applied to all cells in the same row/column is incorrect because after merging cells in a table cells in the same row/column can have different height/width. This means the change in height/width should be applied to every cell that is resized.
Closes #5515
Test plan
I'm currently adding tests for the cases where resizing was breaking previously.
Before
Crash:
Wrong size:
After
Right size:
