Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: close dialog and show toast for Export image #8007

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

yongjoon-km
Copy link

@yongjoon-km yongjoon-km commented May 11, 2024

Save to feature closes dialog and showed toast message after exporting successful.
Export image feature better follow the same flow.

resolves #8008

Save to feature closes dialog and showed toast message
after exporting successful.
Export image feature better follow the same flow.
Copy link

vercel bot commented May 11, 2024

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

Name Status Preview Updated (UTC)
excalidraw ✅ Ready (Inspect) Visit Preview May 15, 2024 6:30am
excalidraw-package-example ✅ Ready (Inspect) Visit Preview May 15, 2024 6:30am
excalidraw-package-example-with-nextjs ✅ Ready (Inspect) Visit Preview May 15, 2024 6:30am
1 Ignored Deployment
Name Status Preview Updated (UTC)
docs ⬜️ Ignored (Inspect) Visit Preview May 15, 2024 6:30am

Copy link
Member

@ad1992 ad1992 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @yongjoon-km for the contribution
Can you also add a toast when copyToClipboard is clicked?

@yongjoon-km
Copy link
Author

Thanks @yongjoon-km for the contribution Can you also add a toast when copyToClipboard is clicked?

Hi @ad1992 , I also apply a toast to the Copy to clipboard. Thank you~

Copy link
Member

@ad1992 ad1992 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @yongjoon-km looks good ❤️

I feel the toast stays for too lon, should we reduce the duration ?
cc @dwelle

@yongjoon-km
Copy link
Author

Hey @yongjoon-km looks good ❤️

I feel the toast stays for too lon, should we reduce the duration ? cc @dwelle

The default duration of the toast is 5 seconds.
I agree with you the toast stays too long. How about 1.5 seconds?
I found the similar duration configuration from here

private onIframeSrcCopy(element: ExcalidrawIframeElement) {
if (element.customData?.generationData?.status === "done") {
copyTextToSystemClipboard(element.customData.generationData.html);
this.setToast({
message: "copied to clipboard",
closable: false,
duration: 1500,
});
}
}

Change the toast duration from 5 seconds to 1.5 seconds.
Copy link
Member

@ad1992 ad1992 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yongjoon-km looking good ✨
I pushed minor tweaks to simplify, let me know if its good

@yongjoon-km
Copy link
Author

@yongjoon-km looking good ✨ I pushed minor tweaks to simplify, let me know if its good

Thanks~! it looks good 👍

@dwelle
Copy link
Member

dwelle commented May 15, 2024

hey @yongjoon-km. Sorry for jumping into this late. To give a bit more context, we've had a request for this once already I believe and there were two reasons why we're not closing the export dialog:

  1. There is a use case where people want to export to two or more formats at once, e.g. PNG + SVG, or PNG but also copy to clipboard for sharing.

    But we are not tracking this, so it's unclear whether this use case is large enough to justify this.

    EDIT: thinking about it more, there may be more common cases like exporting in two different themes (light + dark) or two different scales, etc. So this requires further investigation.

  2. The bigger reason is that we're showing a saving status indicator in the button which is important when exporting larger scenes which can take many seconds (in some cases even 20+ seconds) so that people know what's happening.

    We will start preventing window unload (tab close) as well, but even then we should make it clear that the file is still saving.

    So, even if we decided to ignore the reason (1), we'd at the very least need to come up with an alternative UI for showing the status indicator, before we would be able to merge this.

@dwelle
Copy link
Member

dwelle commented May 15, 2024

Save to feature closes dialog and showed toast message after exporting successful.

That said, no matter if we close the dialog or not, you're making a fair point that the we don't indicate clearly that the saved was successful.

@yongjoon-km
Copy link
Author

yongjoon-km commented May 15, 2024

hey @yongjoon-km. Sorry for jumping into this late. To give a bit more context, we've had a request for this once already I believe and there were two reasons why we're not closing the export dialog:

  1. There is a use case where people want to export to two or more formats at once, e.g. PNG + SVG, or PNG but also copy to clipboard for sharing.
    But we are not tracking this, so it's unclear whether this use case is large enough to justify this.
    EDIT: thinking about it more, there may be more common cases like exporting in two different themes (light + dark) or two different scales, etc. So this requires further investigation.
  2. The bigger reason is that we're showing a saving status indicator in the button which is important when exporting larger scenes which can take many seconds (in some cases even 20+ seconds) so that people know what's happening.
    We will start preventing window unload (tab close) as well, but even then we should make it clear that the file is still saving.
    So, even if we decided to ignore the reason (1), we'd at the very least need to come up with an alternative UI for showing the status indicator, before we would be able to merge this.

Hello @dwelle , Thanks for pointing out previous history on this~

I tried to reproduce large file exporting by delaying inside the exportToSvg function, which I guess creating svg blob file can be delayed when canvas is large. In this case, the dialog I added doesn't just showed up unless file is actually saved.
Can you take a look my implementation executed before file is saved via browser-fs-access?
I think loading button will be waiting until the file is downloaded. If so we need to think about the use case 1 you mentioned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dialog remains after exporting to image
3 participants