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

Expose pageLayoutMode and isFirstPageAlwaysSingle configuration options #46

Merged
merged 5 commits into from
Nov 25, 2019
Merged

Expose pageLayoutMode and isFirstPageAlwaysSingle configuration options #46

merged 5 commits into from
Nov 25, 2019

Conversation

radazzouz
Copy link
Contributor

@radazzouz radazzouz commented Nov 2, 2019 •
edited by simoarpe
Loading

Solves #45

Details

This PR exposes page mode on iOS and page layout mode on Android.

This PR also exposes is first page always single configuration option.

Usage:

Pspdfkit.present(tempDocumentPath, {
  pageLayoutMode: 'automatic',
  iOSIsFirstPageAlwaysSingle: false
});

Screen Shot 2019-11-02 at 11 08 36 AM

Acceptance Criteria

  • The Page Mode configuration option is exposed on iOS.
  • The is first page always single configuration option is exposed on iOS.
  • The Page Layout Mode configuration option is exposed on Android.
  • The is first page always single configuration option is exposed on Android.

@radazzouz radazzouz added enhancement New feature or request Android iOS labels Nov 2, 2019
@radazzouz radazzouz requested a review from aksh1t November 2, 2019 15:16
@radazzouz radazzouz changed the title Expose page mode and is first page always single configuration options Expose pageLayoutMode and isFirstPageAlwaysSingle configuration options Nov 2, 2019
aksh1t
aksh1t approved these changes Nov 4, 2019
View reviewed changes
Copy link
Contributor

@aksh1t aksh1t left a comment

Choose a reason for hiding this comment

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

LGTM!

simoarpe
simoarpe reviewed Nov 11, 2019
View reviewed changes
lib/configuration_options.dart Outdated
const String pageLayoutModeAutomatic = "automatic";
const String pageLayoutModeSingle = "single";
const String pageLayoutModeDouble = "double";
const String iOSIsFirstPageAlwaysSingle = "isFirstPageAlwaysSingle";
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there reasons to make this configuration option iOS specific?
Otherwise, I would extend it Android as well https://pspdfkit.com/api/android/reference/com/pspdfkit/configuration/PdfConfiguration.html#isFirstPageAlwaysSingle()

It would be an easy fix, I can do that: iOSIsFirstPageAlwaysSingle would become isFirstPageAlwaysSingle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure if Android had this property, so I prefixed it with iOS. If we have this property on Android, then I'm all for renaming it as long as we implement it on Android too :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've also just updated the Acceptance Criteria.

@radazzouz radazzouz requested a review from aksh1t November 11, 2019 15:47
simoarpe
simoarpe reviewed Nov 11, 2019
View reviewed changes
example/lib/main.dart Outdated
iOSShowActionNavigationButtonLabels: false
iOSShowActionNavigationButtonLabels: false,
pageLayoutMode: 'automatic',
iOSIsFirstPageAlwaysSingle: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Because this configuration option does not have any effect when in single page mode, it would probably make sense to switch pageLayoutMode to double page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in a6b4b20

lib/configuration_options.dart Outdated
Comment on lines 91 to 92
const String androidSettingsMenuItems = "settingsMenuItems";
const String iOSSettingsMenuItems = "settingsMenuItems";
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ These two (androidSettingsMenuItems and iOSSettingsMenuItems) cannot match with the same string, or the Android part will crash because of the menu items that do not exist in the PSPDFKit for Android. (i.e. just use iOSSettingsMenuItems instead of settingsMenuItems and I'll refactor Android to const String androidSettingsMenuItems = "settingsMenuItems")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Fixed in a6b4b20

Rad Azzouz and others added 3 commits November 11, 2019 14:55
Address feedback on PR
a6b4b20
@simoarpe
Add pageLayoutMode and isFirstPageAlwaysSingle
f718ba8 
Implement pageLayoutMode and isFirstPageAlwaysSingle configuration option for the Android part.

Signed-off-by: Simone Arpe <simon.arpe@gmail.com>
@simoarpe
Enable R8 in the Gradle properties
a935126 
Signed-off-by: Simone Arpe <simon.arpe@gmail.com>
@simoarpe simoarpe added the READY TO REVIEW Pull request ready to review label Nov 12, 2019
@radazzouz
Copy link
Contributor Author

The Android implementation LGTM! 👍

I also retested all the things on both iOS and Android and it works great!

aksh1t
aksh1t approved these changes Nov 22, 2019
View reviewed changes
Copy link
Contributor

@aksh1t aksh1t left a comment

Choose a reason for hiding this comment

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

Looks good!

@radazzouz radazzouz merged commit 70697d5 into master Nov 25, 2019
@radazzouz radazzouz deleted the rad/page-mode branch November 25, 2019 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simoarpe simoarpe simoarpe approved these changes

@aksh1t aksh1t aksh1t approved these changes

Assignees

@simoarpe simoarpe

Labels
Android enhancement New feature or request iOS READY TO REVIEW Pull request ready to review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@radazzouz @aksh1t @simoarpe