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

[contacts] Fix editing contacts on both platforms #27703

Merged
merged 4 commits into from
Mar 15, 2024

Conversation

alanjhughes
Copy link
Collaborator

Why

Noticed some issues with expo-contacts while looking at something else. Editing wasn't working on either platform because one of the arguments is nullable in JS but it wasn't in native so the method calls always failed when null was passed. Also fixed another issue on iOS where sharing was not working correctly.

How

  • Add the correct descriptor on iOS when querying a contact we are going to edit
  • Made the contact argument to presentFormAsync nullable on both platforms
  • Also fixed the icon on the example screens

Test Plan

bare-expo and NCL
All tests are passing and the example screen works correctly now.

@alanjhughes alanjhughes changed the title [contact] Fix editing contacts on both platforms [contacts] Fix editing contacts on both platforms Mar 15, 2024
@expo-bot expo-bot added the bot: needs changes ExpoBot found things that don't meet our guidelines label Mar 15, 2024
@alanjhughes alanjhughes marked this pull request as ready for review March 15, 2024 17:28
@alanjhughes alanjhughes requested a review from EvanBacon as a code owner March 15, 2024 17:28
@expo-bot
Copy link
Collaborator

expo-bot commented Mar 15, 2024

The Pull Request introduced fingerprint changes against the base commit: 95979d9

Fingerprint diff
[
  {
    "type": "dir",
    "filePath": "../../packages/expo-contacts/android",
    "reasons": [
      "expoAutolinkingAndroid"
    ],
    "hash": "129a3e15597a251889f1b7730db954330afec1b0"
  },
  {
    "type": "dir",
    "filePath": "../../packages/expo-contacts/ios",
    "reasons": [
      "expoAutolinkingIos"
    ],
    "hash": "f72977a185d46dcc53ef3bb8b576e49519bb32a8"
  }
]

Generated by PR labeler 🤖

@expo-bot
Copy link
Collaborator

Hi there! 👋 I'm a bot whose goal is to ensure your contributions meet our guidelines.

I've found some issues in your pull request that should be addressed (click on them for more details) 👇

❌ Error: SwiftLint Violations


Found 1 violation, 1 serious. See the review comments below for more insights.


Generated by ExpoBot 🤖 against 816a7aa

@@ -70,22 +70,22 @@ public class ContactsModule: Module {
}
}.runOnQueue(.main)

AsyncFunction("presentFormAsync") { (identifier: String?, data: Contact, options: FormOptions, promise: Promise) in
AsyncFunction("presentFormAsync") { (identifier: String?, data: Contact?, options: FormOptions, promise: Promise) in
Copy link
Collaborator

Choose a reason for hiding this comment

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

🔴 SwiftLint: Closure Body Length Violation

Closure body should span 50 lines or less excluding comments and whitespace: currently spans 53 lines

@alanjhughes alanjhughes merged commit 773ab9e into main Mar 15, 2024
9 of 10 checks passed
@alanjhughes alanjhughes deleted the @alanhughes/contacts/edit-fixes branch March 15, 2024 20:13
promise.resolve()
}

OnActivityResult { _, payload ->
Copy link
Contributor

Choose a reason for hiding this comment

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

@alanjhughes This seems to be a new API in the module definition? Could this be documented in the official docs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@fobos531 It's been there since the beginning. I hadn't realised it was undocumented. Will get that added 👍

@mafo5
Copy link

mafo5 commented Apr 2, 2024

I'm confused about the release of this fix.

I'm currently on the latest version 12.8.2 of the module expo-contacts. This version was released on 20th Dec 2023.
Since then this bug makes it impossible to call the presentForm feature on Android, due to the missing moduleRegistry removed in this fix. On iOS it works well. When does this fix will be released?

@brentvatne brentvatne added the published Changes from the PR have been published to npm label Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: fingerprint changed bot: needs changes ExpoBot found things that don't meet our guidelines published Changes from the PR have been published to npm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants