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

Feature/add should add contact block #35

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

JettF
Copy link

@JettF JettF commented Oct 19, 2016

After working with this library, I realized that I needed a way to filter the contacts based on the the values they had for certain CNKeyDescriptor's. For example, say an app needs to only import contacts which have a valid phone number. By setting the shouldAddContact closure to return true when the phone number for a contact is valid, then you can be sure that the contacts the user can pick from will all have valid phone numbers.

@ipraba: if you don't think this is a good idea, I would love to hear why!

Thanks for your time, JF

@AnthonyMDev
Copy link
Contributor

I'm a contributor on this fork as well. I just fixed a bug in reloadContacts() that was causing duplicate contacts to be shown. It seems that the previous implementation of the method was actually just adding the contacts, not reloading them, so if reloadContact() was called twice, all of your contacts would be duplicated. I've set it up now so that it replaces the loaded contacts properly.

We've also changed the filtering closure name from shouldAddContacts to shouldIncludeContacts. I believe this is a clearer description of what this closure does.

@AnthonyMDev
Copy link
Contributor

This PR should expect no more changes. This has been fully tested and is working for us now in production. If you do want to include this feature, you may merge whenever you are ready. We apologize for the multiple commits post-submission.

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.

2 participants