feat: Add separators on context menu

[Kartik1397]

Fixes #2506 - Check the preview.

This is my first try toward implementing separators in context menu.
One thing which I can improve in this implementation is to use constant varible instead of hard coded numerical value for order and group.
If there is a more clever way to implement this, please suggest me.

Screenshots

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/excalidraw/excalidraw/7f3rlwdec
✅ Preview: https://excalidraw-git-fork-kartik1397-addseparatorsoncontextmenu.excalidraw.now.sh

[lipis]

Visually it looks great ❤️ not sure what to do with the ids though.. needs some thinking :)

[dwelle]

If we don't want to have this weird numbering scheme, another option would be to simply list the actions manually, in the order we want, including separators.

IMO it's not so bad — it'd simplify the code (remove the need for actionManager.getContextMenuItems, item sorting, CANVAS_ONLY_ACTIONS constant, remove group, contextMenuOrder, contextMenuGroupOrder...), and make it more explicit and easier to navigate.

We should also move all the adhoc contextmenu items into proper actions (paste..).

It would look something like this:

import { actionPaste } from "src/actions/actionClipboard"; // this action doesn't exist yet
import { actionSelectAll } from "src/actions/actionSelectAll";

const separator = { type: "separator" };

ContextMenu.push({
	options: [
		actionPaste,
		separator,
		actionSelectAll,
		// ...
	]
});

[Kartik1397]

Thank you @dwelle, your solution seems very nice to me. Removing numbering scheme will definitely improve code quality.

[Kartik1397]

How to convert following methods into actions which are using this.setState() and this.canvas?

  private pasteFromClipboard = withBatchedUpdates(
    async (event: ClipboardEvent | null) => {
      // #686
      const target = document.activeElement;
      const elementUnderCursor = document.elementFromPoint(cursorX, cursorY);
      if (
        // if no ClipboardEvent supplied, assume we're pasting via contextMenu
        // thus these checks don't make sense
        event &&
        (!(elementUnderCursor instanceof HTMLCanvasElement) ||
          isWritableElement(target))
      ) {
        return;
      }
      const data = await parseClipboard(event);
      if (data.errorMessage) {
        this.setState({ errorMessage: data.errorMessage });
      } else if (data.spreadsheet) {
        this.addElementsFromPasteOrLibrary(
          renderSpreadsheet(data.spreadsheet, cursorX, cursorY),
        );
      } else if (data.elements) {
        this.addElementsFromPasteOrLibrary(data.elements);
      } else if (data.text) {
        this.addTextFromPaste(data.text);
      }
      this.selectShapeTool("selection");
      event?.preventDefault();
    },
  );

  private copyToClipboardAsPng = async () => {
    const elements = this.scene.getElements();

    const selectedElements = getSelectedElements(elements, this.state);
    try {
      await exportCanvas(
        "clipboard",
        selectedElements.length ? selectedElements : elements,
        this.state,
        this.canvas!,
        this.state,
      );
    } catch (error) {
      console.error(error);
      this.setState({ errorMessage: error.message });
    }
  };

[dwelle]

How to convert following methods into actions which are using this.setState() and this.canvas?

Actions can (should) return { appState } which is what will be set as new state. We can technically pass canvas to the action, as well.

That said, maybe it's not a good fit for pasteFromClipboard. That function also needs access to ClipboardEvent. Everything is solvable, but maybe we should leave it for later, and instead inline that one/two "actions" into the contextMenu as we've been doing so far? (Sorry for taking you for a ride... 😅)

[lipis]

What's the latest with this one @Kartik1397? Are you stuck anywhere? need help?

[Kartik1397]

@lipis My exam is going on which will end tomorrow. So I will start working on this again from tomorrow.

[lipis]

Priority on the exams! Good luck..

[lipis]

@Kartik1397 are you done with this one?

[Kartik1397]

@lipis Yes, If you fine with not creating action for paste and toggleZenMode for now.

[lipis]

Let's see what @dwelle thinks..

[lipis]

Let's hide the shortcuts at the same breakpoint as with the menu changes.

[dwelle]

Let's hide the shortcuts at the same breakpoint as with the menu changes.

Let's do it in a separate PR as it will require a more involved fix. actually, I hard-coded the fix now, and I'll do the 2nd part in a separate PR.

[dwelle]

@Kartik1397 reviewed. I made a few changes:

1. Change how the canvas is passed. Instead of reusing the formData param which should be used only for the data retrieved from action's accompanying HTML, I added a 4th argument app: App from which we can get canvas directly. This makes it type-safe (which also surfaced a one failure code path — one which in practice wouldn't come up ATM but best be safe).
2. Made toggleZenMode into an action to align with toggleGridMode, and made both helpers in App use those actions.
3. Plus some naming/style tweaks.
4. Reverted the <kbd> → <div> — I think it was a mistake?

If you're ok with the changes, we can merge 🚀

Also, what's your twitter handle?

[lipis]

Let's 🐑 it :)

[Kartik1397]

There was a bug, when we open context menu while cursor type is move.

Before

After

If you're ok with the changes, we can merge

Yeah 👍

Also, what's your twitter handle?

https://twitter.com/ktk_pjpt

[lipis]

🎆