[lexical-playground] 3 Bug Fixes, 1 UX Improvement: All Regarding Excalidraw Node

[neysanfoo]

Description

Bug 1: Excalidraw node being deleted when discarding an empty scene (no elements).

Steps to Reproduce

1. Create some drawing (non-empty)
2. Save the drawing
3. Edit the drawing
4. Clear all the elements
5. Discard Changes

Current Behaviour:

• Drawing gets deleted due to bug in "discard" function that is deleting the node if scene is clear.

New Behaviour:

• Just close the modal. So the changes are actually discarded.

Bug 2: Excalidraw node still being inserted when discarding a non-empty scene.

Steps to Reproduce

1. Create some drawing (non-empty)
2. Discard Changes

Current Behaviour:

• An excalidraw node is still inserted.

New Behaviour:

• Doesn't insert a node.

Video of Bug 1 and 2 (Before Changes)

discard_bug_before.mov

Video of Bug 1 and 2 (After Changes)

discard_bug_after.mov

UX Improvement 1: Do not include the modal in the undo stack

Steps to Reproduce

1. Create some drawing (non-empty)
2. Save the drawing
3. Undo x2
4. Redo

Current Behaviour:

• Firstly, it takes 2 undos to remove the node
• Modal opens up again on first redo

New Behaviour

• Do not add the intermediate steps, i.e. opening the modal, saving changes, or discarding changes, to the undo stack as per Feature: Prevent an action from being added to the undo stack #4283

Video of UX Improvement 1 (Before Changes)

ux_improvement_before.mov

Video of UX Improvement 1 (After Changes)

ux_improvement_after.mov

Bug 3

Lastly, removed a small bug DecoratorNode doesn't have an exportJSON method.

Future Work

• Track the history of resizing, similar to how the image component works

[vercel]

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

Name	Status	Preview	Comments	Updated (UTC)
lexical	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 1, 2024 0:21am
lexical-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 1, 2024 0:21am

[github-actions]

size-limit report 📦

Path	Size
lexical - cjs	29.82 KB (0%)
lexical - esm	29.67 KB (0%)
@lexical/rich-text - cjs	38.42 KB (0%)
@lexical/rich-text - esm	31.51 KB (0%)
@lexical/plain-text - cjs	37.01 KB (0%)
@lexical/plain-text - esm	28.9 KB (0%)
@lexical/react - cjs	40.17 KB (0%)
@lexical/react - esm	32.97 KB (0%)

[etrepum]

It's not really clear why the history-merge tag was added to these updates, can you explain? I think that might not be the desired behavior.

[etrepum]

Nice catch!

[neysanfoo]

It's not really clear why the history-merge tag was added to these updates, can you explain? I think that might not be the desired behavior.

So the history-merge tags that were added are to prevent having the modal in the undo stack (ref to videos of UX improvement). The rest of the code changes are to handle the bugs.

Consider the case: Open modal -> Draw -> Save. I guess there are 2 options for how I can imagine the UX working for undoing and redoing, the first is to count the entire process of opening, drawing and saving as a single action, which is what im proposing. The second option would be 1 undo opens the modal with the current data loaded and then another undo removes the node altogether, and similarly a single redo will open the modal with the data loaded and then another redo will put the image back.

However the current process right now is one undo will delete the image without opening the modal, the second undo will actually delete the node and then a redo will show an empty modal with no data loaded in it, which is a bit weird because we didnt see the modal open when undoing, and also its empty.

So to summarize the history-merge tags were added to the 3 functions to treat the actions of open modal -> draw -> save, open modal -> draw/dont draw -> discard, edit image -> change/no change -> save, edit image -> change/no change -> discard as single actions.

[etrepum]

I am not particularly familiar with this code but I suspect that this might cause some problems with collab which is the only reason I can think of to have the node's state update while the modal is open. In the single user case I think a better strategy would be to only update the state that the decorator exposes to the editor when the modal closes?

[neysanfoo]

I just tried excalidraw using collab. It doesn't seem to be even usable because modal opens on both screens. Would having the toolbar open the excalidraw modal, and then only creating the node on save make sense? I think that would solve both problems.

[etrepum]

I think so, at least that's the approach I would take if I was building that node + plug-in

[neysanfoo]

I have moved the code for opening the modal into the ui folder and decoupled the creation of the node from the opening/editing of the modal. The node is only created on saving the modal.

[neysanfoo]

Any advice how to fix the failing tests?

[ivailop7]

It was the flaky set of tests, all good now. @etrepum happy to merge?

[etrepum]

I tested this out and it the resizing of these nodes does not persist in history, although that is not a regression in this particular PR (maybe the last one though?)

[neysanfoo]

I tested this out and it the resizing of these nodes does not persist in history, although that is not a regression in this particular PR (maybe the last one though?)

I think this has never worked, I'm planning on fixing this next.