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 preserveAspectRatio to CameraOptions while resizing an image #3309

Merged
merged 13 commits into from Jul 24, 2020

Conversation

iphilgood
Copy link
Contributor

This PR is a re-opening of #3297 because of wrong base branch and a retry of #2050 and should fix ionic-team/capacitor-plugins#2 (also the discussion #1952). I've used the Android implementation of @sandstrom (shout-out) and also did an implementation for iOS.

There's now a preserveAspectRatio flag on CameraOptions which is set to false by default.

I've also fixed a bug where the app would crash while saving an image to the gallery on Android. This only happens on certain devices but I was able to reproduce it by creating 32 images until it crashed. A random UUID gets now appended to the file name to make sure no name clashes happen anymore.

@dwieeb is there anything I can do to provide some testing or how do you test this fix?

@imhoffd imhoffd self-assigned this Jul 21, 2020
@imhoffd
Copy link
Contributor

imhoffd commented Jul 21, 2020

@iphilgood To test we usually just add a test button in the example app in the 2.x branch: https://github.com/ionic-team/capacitor/tree/2.x/example

With Capacitor 3 we'll be doing more unit testing. 😁

@iphilgood
Copy link
Contributor Author

@dwieeb I'm gonna fix your reviews tomorrow 👍🏽 Thanks for having a look at it. Are you gonna add the test button or is it me? And do you know why my build failed? I ran the command on my local machine and except for some warnings I didn't see an error 🤔

Hahaha alright 😄 Some tests are always a good idea 😉

@imhoffd
Copy link
Contributor

imhoffd commented Jul 21, 2020

@iphilgood If you could add the button, that would be great! Just making sure there's a new button for using preserveAspectRatio: true and that the other buttons work as expected.

Sometimes the pod lib lint can hang. I just restarted the jobs so we'll see if it passes.

@iphilgood
Copy link
Contributor Author

@dwieeb I've resolved your comments and added a new button to test my code (somehow the menu is not working in the example app. I had to set the RootPage accordingly) 😄

I think the other buttons still work as expected but it would be nice if someone else could have a look at it.

@imhoffd
Copy link
Contributor

imhoffd commented Jul 22, 2020

@iphilgood There isn't a button for the menu but you can swipe from the left of screen because it's just a sidemenu.

This works great on Android and iOS! Nice work!

@imhoffd imhoffd added this to the 2.3.1 milestone Jul 23, 2020
@iphilgood
Copy link
Contributor Author

@dwieeb Hahahaha I'm sorry for that 😅 Now everything makes sense 😂 Thank you!

@sandstrom
Copy link
Contributor

Awesome work @iphilgood 🎉

Copy link
Member

@jcesarmobile jcesarmobile left a comment

Choose a reason for hiding this comment

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

Added two comments.

Other than that, looks good to me.

core/src/core-plugin-definitions.ts Outdated Show resolved Hide resolved
@iphilgood
Copy link
Contributor Author

Okay I've resolved all the comments. Thanks everybody 🤗

Copy link
Member

@jcesarmobile jcesarmobile 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 fixed a typo

Looks good to me

@imhoffd imhoffd merged commit 27a8bcb into ionic-team:2.x Jul 24, 2020
@imhoffd
Copy link
Contributor

imhoffd commented Jul 27, 2020

2.4.0 was just released with this!

@sandstrom
Copy link
Contributor

Awesome, thanks @iphilgood and @dwieeb 🎉

@iphilgood iphilgood deleted the preserve-aspect-ratio-2.x branch July 28, 2020 06:16
@sandstrom
Copy link
Contributor

sandstrom commented Aug 3, 2020

@dwieeb Will there be an update to CHANGELOG.md to include the 2.4.0 changes?

Edit Added PR: #3375

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.

None yet

4 participants