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: choose between exporting with or without annotations #137

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

moonayyur
Copy link
Collaborator

@moonayyur moonayyur commented Jun 23, 2024

closes #119
closes #126

Copy link

cloudflare-workers-and-pages bot commented Jun 24, 2024

Deploying pixelium with  Cloudflare Pages  Cloudflare Pages

Latest commit: 5865214
Status: ✅  Deploy successful!
Preview URL: https://fcab2f67.pixelium.pages.dev
Branch Preview URL: https://119-choose-between-exporting.pixelium.pages.dev

View logs

@moonayyur moonayyur marked this pull request as ready for review June 24, 2024 15:51
Copy link
Contributor

@stropitek stropitek left a comment

Choose a reason for hiding this comment

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

Setting the name in the export to png modal has no effect on the generated file name (the state with the name is not used in the code when saving).

Copy link
Contributor

@stropitek stropitek left a comment

Choose a reason for hiding this comment

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

  • I've put "test" in the name field and the file gets saved as "test.png.png".
  • The default name of the file should be pre-filled in the input and it should not be possible to save with an empty field.

Copy link
Contributor

@stropitek stropitek left a comment

Choose a reason for hiding this comment

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

Would be nice to have some feedback via a toast notification (blueprint has a component for this) once copy to clipboard succeeds / fails.

src/components/modal/ExportClipboardModal.tsx Outdated Show resolved Hide resolved
src/components/modal/ExportPngModal.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@stropitek stropitek left a comment

Choose a reason for hiding this comment

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

If no image is selected, the export toolbar should disappear.

Comment on lines +97 to +100
if (!hasAnnotations) {
save();
return null;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

My suggestion was not just to remove the effect (it was correct to use an effect in that case), but to move this logic up to the component that handles the export options, which will prevent having to use an effect because then it can be called in an event callback.

copyToClipboard().catch((error) =>
logger.error(`Failed to copy to clipboard: ${error}`),
);
openExportClipboardModal();
Copy link
Contributor

Choose a reason for hiding this comment

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

Here in the select callback there can be additional logic to handle the cases where the image has / does not have annotations.

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.

Improve name of exported file Choose between exporting with or without annotations
2 participants