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.

Sign up for GitHub

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

Jump to bottom

Add SaveAsHandler for custom save as behavior #1000

Open
wants to merge 5 commits into
base: 8.11
Choose a base branch
from
Open

Add SaveAsHandler for custom save as behavior #1000

wants to merge 5 commits into from

Conversation

uwohlfeil
Copy link

This pull request is a proposal for an extended functionality in the area of the 'save as' functionality.

Currently the saveAs function of the file-saver library is always triggered here. In some cases, it may be useful to implement this event individually in the context of customizing. E.g. in the context of a document management system, the result should not be saved locally but stored directly as a new file in the DMS. There can also be problems in the context of the IFrame attribute sandbox (IFrame in IFrame case).

See also:
https://community.apryse.com/t/intercept-overwrite-download-function-in-webviewer/7245

WebViewer(...)
  .then(function(instance) {
    // data: Blob|File , filename: string 
    function saveAsHandler(data, filename) {
      // Create  download Link, Open Tab, call individual api
    };

    instance.UI.setSaveAsHandler(saveAsHandler);
  });

@bollain
Copy link
Collaborator

bollain commented Feb 16, 2023

Hi @uwohlfeil,

Thanks for making this PR! We will add this to our following sprint to review. Our next sprint starts next Monday. Once we have completed our review we will provide feedback.

Thanks for taking the time to submit this, you will hear from us soon.

@uwohlfeil
Copy link
Author

I have added a few corrections in the area of the German translation:

Incorrect translations

  • action.addMark and pageRedactModal.addMark
  • old: Markus hinzufügen
  • new: Markieren (Long will be Markierung hinzufügen, but we think that will break the layout)
  • action.insertBlankPageBelow
    • old: Leere Seite unten einfügen page
    • new: Leere Seite unten einfügen"
  • redactionPanel.clearMarked
  • old: Klar (Clear like clear water, clear view and so on)
  • new: Abbrechen (cancel). The longer correct translation will be like Markierungen entfernen, but this may break the layout

Optimization

This translations are not wrong but we think they can be optimized.

  • action.selectAll
    • old: Wählen Sie Alle (sounds like a command)
    • new: Alle auswählen
  • redactionPanel.redactionSearchPlaceholder
    • old: Hinzufügen nach Schlüsselwortsuche oder Mustern
    • new: Schlüsselwortsuche oder Muster
  • settings.lightMode
    • old Lichtmodus not wrong but sound strange. The english version is very common in german
    • new Light-Mode
  • settings.darkMode
    • old Dunkler Modus works but we think the english version is very common in german
    • new Dark-Mode
  • settings.advancedSetting
    • old Erweiterte Einstellung
    • new Erweitert this text fits better (but the original works but then Erweiterte Einstellungen)
  • settings.disableFadePageNavigationComponent
    • old Deaktivieren Sie die Seitennavigationskomponente ausblenden sounds like a command
    • new Seitennavigationskomponente nicht automatisch ausblenden
  • settings.disableToolDefaultStyleUpdateFromAnnotationPopup
    • old Deaktivieren Sie die Aktualisierung des Werkzeug-Standardstils aus dem Anmerkungs-Popup - Command
    • new Aktualisierung des Werkzeug-Standardstils aus dem Anmerkungs-Popup deaktivieren"

Ideas

The toolbar names are a little bit confusing and very long.

  • option.toolbarGroup.toolbarGroup-FillAndSign
    • old Füllen und unterschreiben
    • new Formular (Forms)
  • option.toolbarGroup.toolbarGroup-Forms
    • old Formen
    • new Zeichnen (Draw/Sketch)

@uwohlfeil
Copy link
Author

We currently have the problem that the thumbnail multiselect checkbox input control is slightly shifted to the right. This makes it impossible to select the checkbox in the left part of the framed area. The problem seems to be the css rule "position: absolute". We suspect that our solution is not correct, but we hope that it shows the problem.

Before:
grafik

After:
grafik

carlopdftron
carlopdftron requested changes Mar 1, 2023
View reviewed changes
Copy link
Contributor

@carlopdftron carlopdftron left a comment

Choose a reason for hiding this comment

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

Thank you for you feedback and additions! Most of this looks good. For the checkbox issue, we are working on implementing a fix already so you can leave the css change out for now. The only other changes would be to use const instead of var in var handler = getSaveAsHandler();

src/components/Choice/Choice.scss Outdated
@@ -10,6 +10,10 @@
align-items: center;
}

.ui__choice__input input {
Copy link
Contributor

Choose a reason for hiding this comment

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

We have investigated the issue causing the checkboxes to go off-centre. We'll be implementing a fix for this soon. This particular CSS selector might affect other inputs so it would be best to leave this out for now.

src/components/FileAttachmentPanel/FileAttachmentPanel.js Outdated
@@ -53,7 +54,12 @@ const FileAttachmentPanel = () => {
{fileAttachments.embeddedFiles.map((file, idx) => renderAttachment(
file.filename,
() => {
saveAs(file.blob, file.filename);
if (getSaveAsHandler() !== null) {
var handler = getSaveAsHandler();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be a const here instead of a var

src/components/ReplyAttachmentList/ReplyAttachmentList.js Outdated
@@ -67,7 +68,12 @@ const ReplyAttachmentList = ({ files, isEditing, fileDeleted }) => {
e.stopPropagation();

const fileData = file.url ? file.url : await decompressFileContent(file);
saveAs(fileData, file.name);
if (getSaveAsHandler() !== null) {
var handler = getSaveAsHandler();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be a const here instead of a var

src/event-listeners/onFileAttachmentDataAvailable.js Outdated

export default () => (fileMeta) => {
const { fileData, fileName } = fileMeta;
saveAs(fileData, fileName);
if (getSaveAsHandler() !== null) {
var handler = getSaveAsHandler();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be a const here instead of a var

src/helpers/downloadPdf.js Outdated
@@ -238,7 +239,12 @@ export default async (dispatch, options = {}, documentViewerKey = 1) => {
} else {
file = new File([arr], downloadName, { type: downloadType });
}
saveAs(file, downloadName);
if (getSaveAsHandler() !== null) {
var handler = getSaveAsHandler();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be a const here instead of a var

src/helpers/pageManipulationFunctions.js Outdated
@@ -78,12 +79,22 @@ const extractPages = (pageNumbers, dispatch) => {
title,
confirmBtnText,
onConfirm: () => extractPagesWithAnnotations(pageNumbers).then((file) => {
saveAs(file, 'extractedDocument.pdf');
if (getSaveAsHandler() !== null) {
var handler = getSaveAsHandler();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be a const here instead of a var

src/helpers/pageManipulationFunctions.js Outdated
}),
secondaryBtnText,
onSecondary: () => {
extractPagesWithAnnotations(pageNumbers).then((file) => {
saveAs(file, 'extractedDocument.pdf');
if (getSaveAsHandler() !== null) {
var handler = getSaveAsHandler();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be a const here instead of a var

@uwohlfeil
Copy link
Author

Thank you for you feedback and additions! Most of this looks good. For the checkbox issue, we are working on implementing a fix already so you can leave the css change out for now. The only other changes would be to use const instead of var in var handler = getSaveAsHandler();

@carlopdftron
Thank you for your feedback and that you liked the changes.
I tried to correct the noted points accordingly.

carlopdftron
carlopdftron requested changes Mar 3, 2023
View reviewed changes
Copy link
Contributor

@carlopdftron carlopdftron left a comment

Choose a reason for hiding this comment

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

After further review, it would be better to use the same model that we use for other APIs to get/set the saveAsHandler with Redux.

Please refer to 101fb32 to see how we use customApplyRedactionsHandler to do something similar to this.

This way, there would be no need for the saveAs helper functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlopdftron carlopdftron carlopdftron requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@uwohlfeil @bollain @carlopdftron