-
Notifications
You must be signed in to change notification settings - Fork 620
fix: booleans on ios and missing prop #690
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
fix: booleans on ios and missing prop #690
Conversation
cixio
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works great. Thanks!
BrendonGomes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This solution works fine on RN 0.76.5 IOS 18.1 emulator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function worked smoothly in the android emulator
| id maxPhotoQualityPrioritization = CKConvertFollyDynamicToId(newProps.maxPhotoQualityPrioritization); | ||
| if (maxPhotoQualityPrioritization != nil && [maxPhotoQualityPrioritization isKindOfClass:NSString.class]) { | ||
| _view.maxPhotoQualityPrioritization = [flashMode isEqualToString:@"balanced"] ? CKMaxPhotoQualityPrioritizationBalanced : [flashMode isEqualToString:@"quality"] ? CKMaxPhotoQualityPrioritizationQuality : CKMaxPhotoQualityPrioritizationSpeed; | ||
| [changedProps addObject:@"maxPhotoQualityPrioritization"]; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still uses flash mode.
I would have preferred regular if-statements here since this is a little rough to read. Or at least some variables to ensure it was more readable and succinct.
Can you record a video to show that it's working/not crashing on both platforms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@WoLewicki can you please get this video uploaded so we can all use this great fix?
|
Pleeeeeease get this through I need this so badly |
|
@fluffydonkey you can always use patch-package to introduce those changes to your project. I will look at this now though. |
|
Here is a video with the prop being swapped since there is no button for it. photoQuality.mp4 |
|
Thank you for the PR and your help/collaboration on the related tickets @WoLewicki ! |
|
@scarlac @WoLewicki this PR introduced a regression that causes Android builds on RN 0.77 (old arch) to fail with the following error: /node_modules/react-native-camera-kit/android/src/main/java/com/rncamerakit/CKCameraManager.kt:22:83 Argument type mismatch: actual type is 'com.rncamerakit.CKCameraManager', but 'U!' was expected.which had previously been fixed in #695. |
|
@d-moreira Good catch. I confirmed it locally. Fix released in v15.0.1 now. |
|
Probably it was added because that is that's how it's now done by codegen. Maybe introducing sourcesets for this would be the best option. wdyt @scarlac ? |
Summary
PR fixing how booleans are handled on new arch since using the folly converter makes them resolve to
trueeverytime. It also adds missing prop.