fix: Add multiElement-edit finalize action to Desktop (currently only visible in Mobile view)

[zsviczian]

The multielement finalize action is only displayed on the Mobile view, however multielement edit can be initiated on iPad, or touchscreens as well, but without the finalize action, the multielement editing cannot be finalize.

Before

I can’t stop editing the line:

IMG_0152.MOV

After

I added the finalize button next to undo in the bottom left.

IMG_0153.MOV

[vercel]

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

excalidraw – ./

🔍 Inspect: https://vercel.com/excalidraw/excalidraw/EXDuZrT85wGqPTR8R4aWsHhPJdB5
✅ Preview: https://excalidraw-git-zsviczian-fix-multielement-finalize-excalidraw.vercel.app

excalidraw-package-example – ./src/packages/excalidraw

🔍 Inspect: https://vercel.com/excalidraw/excalidraw-package-example/Ef4819wAkWPdUP4w2XMggXYbhaQp
✅ Preview: https://excalidraw-package-example-git-zsviczian-fix-e00637-excalidraw.vercel.app

[zsviczian]

@dwelle, I think this is a straight forward and important fix. We should merge it.

In an unlucky situation (I experienced this first hand many times), you can accidentally start editing a multielement line or arrow on a touch screen when you touch the screen with two fingers. On a touchscreen (without this fix) there is no way to gracefully exit the editing (double tap does not work). The only way out is to close and reopen excalidraw... or as by now I've learned, by closing the line by tapping on the first point... but this is not very intuitive.

Do you see any issue with the proposed fix?

[zsviczian]

Hi @ad1992, @dwelle,
I wanted to bring this PR to your attention once more. This is an easy fix for tablets, where, if you start to create a multielement with two fingers, there is no way to stop editing because the finalize button is not displayed. The finalize button is only displayed when isMobile is true.
If you'd want to hide the finalize button for non-touch screens, we could implement the isTouchScreen approach here as well. However, I don't see the negative effect of displaying the finalize check button on non-touchscreens.

[ad1992]

Sure lets show this for isTouchScreen, for desktop devices its not needed so lets not add it and keep the UI minimal

[dwelle]

@zsviczian sorry for missing this. I somewhat agree with @ad1992 — while I think we could leave it for desktop just fine (it's contextual), the UX isn't the best when using a mouse or any pointing device which actually moves the cursor across canvas, because it will preview the last point all the way until you click the button.

That said, maybe the bad UX is worth the added clarity for those who would otherwise be stuck in the creation mode (i.e. those who don't read tooltips or cannot use keyboard for some reason. Also those whom we incorrectly detect as !isTouchScreen)

[ad1992]

@zsviczian sorry for missing this. I somewhat agree with @ad1992 — while I think we could leave it for desktop just fine (it's contextual), the UX isn't the best when using a mouse or any pointing device which actually moves the cursor across canvas, because it will preview the last point all the way until you click the button.

That said, maybe the bad UX is worth the added clarity for those who would otherwise be stuck in the creation mode (i.e. those who don't read tooltips or cannot use keyboard for some reason. Also those whom we incorrectly detect as !isTouchScreen)

As a user I just click on the same point to stop editing. As per the tooltips, if people are using multi point editing, there is a high chance they would have read the tooltip to get started otherwise its a tendency to draw a line when not reading tooltip. Also we shouldn't add another action coz we are assuming that user might miss tooltips and instead make the tooltip better, and the space could be utilised for hidden actions which are available only via keyboard eg "zoom to fit" so for desktop users I still think we should avoid showing it unless there is an explicit need

[zsviczian]

As a user I just click on the same point to stop editing. As per the tooltips, if people are using multi point editing, there is a high chance they would have read the tooltip to get started otherwise its a tendency to draw a line when not reading tooltip. Also we shouldn't add another action coz we are assuming that user might miss tooltips and instead make the tooltip better, and the space could be utilised for hidden actions which are available only via keyboard eg "zoom to fit" so for desktop users I still think we should avoid showing it unless there is an explicit need

So I am one of those users who missed reading the tooltip 😳 and struggled to finalize the line I started with two fingers on my iPad... Why do we have the finalize button on the mobile toolbar? BTW I missed that as well, until @dwelle brought it to my attention. In short the learning might be that I should pay more attention to tooltips, but also that this function is somehow not intuitive (at least not for me)... I will make the change to only show the finalize button if the user initiated the action with a touch.

[ad1992]

As a user I just click on the same point to stop editing. As per the tooltips, if people are using multi point editing, there is a high chance they would have read the tooltip to get started otherwise its a tendency to draw a line when not reading tooltip. Also we shouldn't add another action coz we are assuming that user might miss tooltips and instead make the tooltip better, and the space could be utilised for hidden actions which are available only via keyboard eg "zoom to fit" so for desktop users I still think we should avoid showing it unless there is an explicit need

So I am one of those users who missed reading the tooltip 😳 and struggled to finalize the line I started with two fingers on my iPad... Why do we have the finalize button on the mobile toolbar? BTW I missed that as well, until @dwelle brought it to my attention. In short the learning might be that I should pay more attention to tooltips, but also that this function is somehow not intuitive (at least not for me)... I will make the change to only show the finalize button if the user initiated the action with a touch.

Yes definitely the tooltips are not great right now and could be easily missed thats why we should make the tooltips better for sure and we will soon as well :). What I meant is on Desktop there is a very less chance that user would start multi point without looking at tooltip as the general tendency is to draw a line otherwise I feel. And I do agree the finalise button is not a great UX as well so once we redesign the editor this will be taken care as well.

[dwelle]

The double-tap to finalize isn't a good UX on mobile/touch, so we'll want the finalize button no matter if people read tooltips or not.

Though I must say the current positioning of the finalize button next to the undo buttons will most likely be missed by users too.

[ad1992]

The double-tap to finalize isn't a good UX on mobile/touch, so we'll want the finalize button no matter if people read tooltips or not.

Though I must say the current positioning of the finalize button next to the undo buttons will most likely be missed by users too.

Yes thats why I said lets have the finalise button (current one) for mobile/touch devices only as I don't see the need for desktop devices, later on when we redesign we can have it for all devices if needed

[ad1992]

What's the status of this?

[zsviczian]

I haven't had time to updated the PR with the isTouchScreen check. Hope to get to it this weekend.

…

On Wed, Feb 23, 2022, 07:33 Aakansha Doshi ***@***.***> wrote: What's the status of this? — Reply to this email directly, view it on GitHub <#4764 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADNRO6WSWAEXV3XOTRUYMLDU4R5TBANCNFSM5N5RIBZQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[zsviczian]

@dwelle , seems @ad1992 and I are happy with this change. Do you approve?

[dwelle]

Mutating the app.deviceType object will make the context not notify observers. It "works" now by virtue of the dependent components being updated by other means, but it makes the API prone to regressions or not working for other components in the future.

But instead of creating the object (renewing the reference) every time unnecessarily, we should do it only on actual change. Thus make a util function like this:

(basically the same api as newElementWith())

export const updateObject = <T extends Record<string, any>>(
  obj: T,
  updates: Partial<T>,
): T => {
  let didChange = false;
  for (const key in updates) {
    const value = (updates as any)[key];
    if (typeof value !== "undefined") {
      if (
        (obj as any)[key] === value &&
        // if object, always update because its attrs could have changed
        (typeof value !== "object" || value === null)
      ) {
        continue;
      }
      didChange = true;
    }
  }

  if (!didChange) {
    return obj;
  }

  return {
    ...obj,
    ...updates,
  };
};

And use it as this.deviceType = updateObject(this.deviceType, { isTouchScreen: true })

[zsviczian]

Mutating the app.deviceType object will make the context not notify observers. It "works" now by virtue of the dependent components being updated by other means, but it makes the API prone to regressions or not working for other components in the future.

@dwelle, Thanks!

I learned something today.
Please review and let me know if you are now ok to merge this into master. Once this is merged, as a next PR, I will add penMode to device type.

[ad1992]

@zsviczian wanna create a video for the same for the tweet?

[zsviczian]

@ad1992, sure, I’ll make a short video and a tweet later tonight.