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: exporting redesign #3613

Merged
merged 22 commits into from
May 25, 2021
Merged

feat: exporting redesign #3613

merged 22 commits into from
May 25, 2021

Conversation

dwelle
Copy link
Member

@dwelle dwelle commented May 21, 2021

  • Two export dialogs
    • json-export dialog — everything related to json (save to file, save as link, later save to Excalidraw+)
    • image-export dialog — for exporting to image (PNG/SVG/copy-png-to-clip)

package API updates in next PR

TODOS

image

image

@vercel
Copy link

vercel bot commented May 21, 2021

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/7596DKtoqcN8JwYP4cXpa2vdEJDU
✅ Preview: https://excalidraw-git-exportredesign-excalidraw.vercel.app

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.

looking 🔥
This is the first dialog whose content has multiple colors :)

src/components/JSONExportDialog.tsx Outdated Show resolved Hide resolved
Comment on lines 49 to 52
style={{
display: "grid",
gridTemplateColumns: "repeat(auto-fit, minmax(200px, 1fr))",
justifyItems: "center",
Copy link
Member

Choose a reason for hiding this comment

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

lets not use inline style, it makes difficult to override css in consumers (boils down to using !important)

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved into a class in this instance, but I wouldn't want the above be a consideration. Once we start using utility classes hosts will need to revert to hacks anyway. If we want to support customizing CSS we should have an explicit API/workflow.

@@ -382,7 +383,29 @@ const LayerUI = ({
}: LayerUIProps) => {
const isMobile = useIsMobile();

const renderExportDialog = () => {
const renderJSONExportDialog = () => {
if (!UIOptions.canvasActions.export) {
Copy link
Member

Choose a reason for hiding this comment

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

we will need to rename this to exportToJSON, and for image exportIToImage, this can be done in the next pr.

>
<Card color="lime">
<div className="Card-icon">{exportToFileIcon}</div>
<h2>Save to disk</h2>
Copy link
Member

Choose a reason for hiding this comment

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

Lets either use Export/ save instead of both depending on the icon we finalize

Copy link
Member Author

Choose a reason for hiding this comment

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

I purposefully used both not to repeat ourselves + it never hurts to use different terms so as to explain a concept more fully (and help people associate the two words together).

@vjeux
Copy link
Contributor

vjeux commented May 21, 2021

right now this will just serve to export images by we may generalize it later (e.g. save to PDF), hence canvas naming instead of image. Note: This is just internal naming.

Super nitpick but we're not using canvas for exporting to svg :p I think image is actually more representative where we're exporting the actual image in different shapes rather than the json. But doesn't really matter :p

@vjeux
Copy link
Contributor

vjeux commented May 21, 2021

I like the structure of it more than the current one. Really excited about it, thanks for driving this.

@vjeux
Copy link
Contributor

vjeux commented May 21, 2021

It made me curious to look at the export to image dialog:
image

I think it's not super clear what happens here. I feel like it should be restructured a bit to make it clear that the structure is:

[Preview]
[Options] -> 1/2/3x, made by with...
[Call to Action] -> export as png, svg, clipboard

Don't have a clear idea of how to do that but I think it'd be great.

Maybe we need to use the fancy colors for the buttons :p

@dwelle
Copy link
Member Author

dwelle commented May 21, 2021

Super nitpick but we're not using canvas for exporting to svg :p I think image is actually more representative where we're exporting the actual image in different shapes rather than the json. But doesn't really matter :p

Yeah, by canvas I didn't mean canvas as HTMLCanvasElement, but rather the canvas content :), but we can indeed rename it as it obviously creates confusion.

@dwelle
Copy link
Member Author

dwelle commented May 22, 2021

I noticed that "export only selected" also applies the shareable link. Didn't know about this, and I think it's not intuitive. It should work the same as save-to-disk works. As such, I'll remove this from this PR (i.e. not introduce that checkbox into the JSON export dialog at all).

@dwelle
Copy link
Member Author

dwelle commented May 22, 2021

I added the filename input to JSON export dialog, but show it only when the browser doesn't support native-FS. The reason is the the native-FS doesn't reuse the filename anyway, and we're actually keeping the input only due to legacy browsers which aren't showing the save dialog at all (and thus users don't have a way to rename their files prior to saving).

Added it into the save-to-disk card because it'd not work well on mobile when the cards are wrapped.

image

@dwelle
Copy link
Member Author

dwelle commented May 22, 2021

I've hidden the filename from image export dialog when native-FS is supported, for the same reason.

@dwelle
Copy link
Member Author

dwelle commented May 22, 2021

Not sure what title to use for the json export. Current export alone may not work:

image

@ad1992
Copy link
Member

ad1992 commented May 22, 2021

Not sure what title to use for the json export. Current export alone may not work:

image

why not save ?

@dwelle
Copy link
Member Author

dwelle commented May 22, 2021

why not save ?

Not sure it would work with shareable-links, and later E+ export.

@ad1992
Copy link
Member

ad1992 commented May 22, 2021

why not save ?

Not sure it would work with shareable-links, and later E+ export.

Hmm in that case how about `"Save as image" instead of Export to image ? and keep export as it is

@ad1992
Copy link
Member

ad1992 commented May 23, 2021

It's true that they are similar, but I realized it may not be an issue as people will at least will explore both
dialog, and since both are exporting things I thought it best to use arrows. Another consideration is that an image alone may evoke image importing, which would further be exacerbated once we add actual support for it and add another image icon.

Agreed, as discussed let's keep the arrow and we can improve it later always.

What do you think about the texts in the JSON export dialog? Do you see anything you'd change or add?

Looks good 👍🏻

@dwelle
Copy link
Member Author

dwelle commented May 23, 2021

As a temporary solution I'm showing the save-to-current-file button next to the darkMode toggle, provided there's an active file handle:

image

We can later figure out a different placing, such as the one suggested in #3381

@dwelle dwelle marked this pull request as ready for review May 24, 2021 12:57
@ad1992
Copy link
Member

ad1992 commented May 24, 2021

As a temporary solution I'm showing the save-to-current-file button next to the darkMode toggle, provided there's an active file handle:

image

We can later figure out a different placing, such as the one suggested in #3381

Yep sounds good 👍🏻 , Once the tooltip pr is merged will do a final review

# Conflicts:
#	src/actions/actionExport.tsx
#	src/components/Tooltip.scss
src/components/Card.tsx Show resolved Hide resolved
src/components/Card.tsx Show resolved Hide resolved
src/components/ExportDialog.scss Outdated Show resolved Hide resolved
src/components/ImageExportDialog.tsx Show resolved Hide resolved
src/components/ImageExportDialog.tsx Outdated Show resolved Hide resolved
src/components/ImageExportDialog.tsx Outdated Show resolved Hide resolved
aria-label={t("buttons.exportToPng")}
onClick={() => onExportToPng(exportedElements, scale)}
>
PNG
Copy link
Member

Choose a reason for hiding this comment

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

i18n

Copy link
Member Author

Choose a reason for hiding this comment

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

This comes from production where we don't use i18n for these image labels and I think that makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm not sure about this coz since these are labels it should be readable as "PNG/SVG" only but in all langs, since it's in production so we can discuss this separately.

src/components/ImageExportDialog.tsx Show resolved Hide resolved
src/components/Card.scss Outdated Show resolved Hide resolved
src/components/Card.scss Outdated Show resolved Hide resolved
Co-authored-by: Aakansha Doshi <aakansha1216@gmail.com>
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.

lets ship it 🎉

@dwelle
Copy link
Member Author

dwelle commented May 25, 2021

Yay! 🚀

@dwelle dwelle merged commit 790c9fd into master May 25, 2021
@dwelle dwelle deleted the export_redesign branch May 25, 2021 19:37
@sebasjm
Copy link

sebasjm commented May 26, 2021

I came here because I didn't find the export to SVG with embed scene.

My 2 cents, this new look is better but... since SVG with embed scene is the only that allows you use it in html and still being able to modify it IMHO it should be the default choice and now it looks like a deep secret.

Any other format feels an optimization for me.

Anyways... awesome work!

@dwelle
Copy link
Member Author

dwelle commented May 26, 2021

hey @sebasjm, can you elaborate on it? As I understand it you did find it in the end, right? What was the biggest issue — find the actual image export dialog itself? The iconography could indeed be better and we may change it later.

We were trying to optimize for 1) intuitiveness in terms of grouping related formats together (e.g. previously we've had link-sharing inside image export dialog which didn't make that much sense and was rather hidden), and 2) making the export buttons more prominent and giving additional info (e.g. again for the link-sharing).

@sebasjm
Copy link

sebasjm commented May 26, 2021

Well... after seeing the new "save to disk" dialog I didnt want to believe that this feature was missing so I didnt surrender :P
I came here first looking for issues and then looking for PR with "svg", I found this PR but I had to look into the conversation to find out that it wasnt missing, just moved. I'm not saying that no one will find it out but I didnt.

I follow your logic about where the button ended up and I'm ok with that, maybe it is not intuitive if you were used to have it in other place.

If I have my chance to add a comment about the buttons: IMHO there should be 2

And change "embed scene" to "remove scene from file" with a warning that you will reduce size but not be able to load it again.

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.

None yet

4 participants