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

Add form editing configuration #365

Merged
merged 9 commits into from
Apr 27, 2020

Conversation

tomassurin
Copy link
Contributor

@tomassurin tomassurin commented Apr 23, 2020

Details

Adds enableFormEditing: boolean to Android ConfigurationAdapter class. This maps to PSPDFKit's PdfActivityConfiguration.Builder#enable/disableFormEditing().

Acceptance Criteria

  • Add JavaScript API for disabling form editing on Android.
  • Add JavaScript API for disabling form editing on iOS.
  • When approved, right before merging, rebase with master and increment the package version in package.json, package-lock.json, samples/Catalog/package.json, and samples/NativeCatalog/package.json (see example commit: 1bf805f).
  • Create a new release (and tag) with the new package version (see https://github.com/PSPDFKit/react-native/releases).

@radazzouz radazzouz added the iOS label Apr 23, 2020
@radazzouz radazzouz requested a review from steviki April 23, 2020 17:22
}

@NonNull
private PdfActivityConfiguration updateReactPropsInConfiguration(@NonNull PdfActivityConfiguration configuration) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reasoning behind this method is that iOS defines the enableFormEditing property as well as the configuration. Since the order of react props application is not guaranteed, we need to make sure that the property value is not overriden by the configuration.

Copy link
Contributor

Choose a reason for hiding this comment

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

On iOS, the last used option overrides the previous one. For example, if the user sets a value in the configuration, and they later override it in the prop, we will use the most recent one.

index.js Outdated
/**
* Controls whether or not the document's forms are editable. Defaults to forms being editable (true).
*/
enableFormEditing: PropTypes.bool,
Copy link
Contributor Author

@tomassurin tomassurin Apr 23, 2020

Choose a reason for hiding this comment

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

This leads to confusing API on Android where this property has higher importance than the configuration.enableFormEditing (see my comment above https://github.com/PSPDFKit/react-native/pull/365/files#r413992917). Is this really needed for iOS?

Copy link
Contributor

@radazzouz radazzouz Apr 24, 2020

Choose a reason for hiding this comment

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

No, the view prop is no longer required.

index.js Outdated
/**
* Controls whether or not the document's forms are editable. Defaults to forms being editable (true).
*/
enableFormEditing: PropTypes.bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

Disabling this on iOS not only disables form editing, but also form viewing. Meaning that all forms (including their set value or contents) are essentially hidden.
Is this the desired behavior and does this match the Android behavior?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point! I'll update the iOS implementation if needed to match the Android behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On Android this only disables form editing. Widget annotations are still rendered.

The only way I know of to disable form rendering is when the acroforms license is missing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the Android implementation makes more sense, and also matches what I would expect from enableFormEditing.

@radazzouz On iOS we could use editableAnnotationTypes to exclude widgets to get the same behavior I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you both for the info. I just pushed the changes in 4da5755.

Copy link
Contributor

@steviki steviki left a comment

Choose a reason for hiding this comment

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

One question about the desired behavior, but apart from that, the iOS part looks good to me!

@radazzouz radazzouz requested a review from steviki April 24, 2020 15:09
Copy link
Contributor

@steviki steviki left a comment

Choose a reason for hiding this comment

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

LGTM!

@tomassurin tomassurin merged commit df12320 into master Apr 27, 2020
@tomassurin tomassurin deleted the tomas/add-enable-form-editing-config branch April 27, 2020 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants