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

Update imports for the latest version #83

Merged
merged 5 commits into from
Feb 9, 2021

Conversation

radazzouz
Copy link
Contributor

@radazzouz radazzouz commented Feb 8, 2021

Solves https://github.com/PSPDFKit/PSPDFKit/issues/28037


Details

What this PR does:

  • Fixes import in getting started code snippets for iOS and Android.
  • Updates the requirements in the iOS README.
  • Adds “Migrating from Version 1.10.3” section in the main README.

Acceptance Criteria

  • Before merging, retest all the examples to make sure that they work on both Android and iOS.
  • Before merging, make sure that everything works in a newly created Flutter project on both Android and iOS.

@radazzouz radazzouz self-assigned this Feb 8, 2021
@radazzouz radazzouz added Android bug Something isn't working iOS READY TO REVIEW Pull request ready to review labels Feb 8, 2021
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.

Thank you for fixing this, Rad!

I tested the android example from the readme, and it launches - however I am getting this error: File not found or could not be opened. when I open the file. I double checked that the file is there and that the app was granted permission, not sure what else could be wrong.

Copy link
Contributor

@amit-nayar amit-nayar left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for fixing this.
Just a comment on running upgrade.
Also hopfully issues like this can be removed if we take the example code out of the readme and instead focus on example projects which we could pin to a pspdfkit-flutter version.
Seems pretty error prone to have to copy code out of the readme.

@@ -87,7 +87,7 @@ android {
```dart
Copy link
Contributor

Choose a reason for hiding this comment

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

I have an issue with get packages where flutter was grabbing an old version of our plugin.
I had to run flutter packages upgrade to upgrade to latest. I think this would only be an issue for updating an existing project but not 100%.
Might be worth adding flutter packages upgrade as a step after flutter packages get in the readme to make sure users don't fall into the same trap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@amit-nayar the flutter packages upgrade was required when you created a new Flutter project?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, which is why I'm not sure it's necessary in the readme. I thought it might help though for users who may be upgrading their existing projects. But honestly not sure in this case. It should probably be common knowledge for Flutter users. WDYT?

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 did not have to run the command when I created a new project to test this fix. So I would avoid adding an extra integration step if we can avoid it.

@amit-nayar
Copy link
Contributor

@radazzouz can you add a changelog entry for this as well? In the monorepo merge I'll add a danger check so we won't need to remember each time 😄

@radazzouz radazzouz merged commit 65ef1f0 into master Feb 9, 2021
@radazzouz radazzouz deleted the rad/fix-getting-started-code-snippet branch February 9, 2021 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android bug Something isn't working iOS READY TO REVIEW Pull request ready to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants