Skip to content
This repository was archived by the owner on May 26, 2019. It is now read-only.

Conversation

@aarkalyk
Copy link
Contributor

No description provided.

@sujameslin
Copy link
Contributor

@aarkalyk Would you please review the feedback that I left?

@aarkalyk
Copy link
Contributor Author

aarkalyk commented Feb 23, 2018

@sujameslin Sorry, mate, I can't see any feedback. Could you, please, send me the link?

@sujameslin
Copy link
Contributor

@aarkalyk
Weird. Not sure why you can't see my feedback in this PR.
Anyway I attached screenshot which will help you.
screen shot 2018-03-02 at 9 23 09 pm

Also would you please elaborate README.md to include your localizedCallerName params?
Thanks

@aarkalyk
Copy link
Contributor Author

aarkalyk commented Mar 2, 2018

@sujameslin It's indeed weird that I can't see your comments. Thanks for the screenshot! I'll updated the PR soon.

@aarkalyk
Copy link
Contributor Author

aarkalyk commented Mar 2, 2018

here's how contactIdentifier is described in the docs

The identifier is displayed in the native call UI, and is typically the name of the call recipient.
If a caller corresponds to a CNContact object, set this to the value of the identifier property of the contact.

Although, it can be used as a name as well, it's not the same thing as localizedCallerName. As we can see it can be used as a contact identifier too.

A value that uniquely identifies a contact on the device.

I don't feel comfortable renaming it as localizedCallerName, I think description in README should be enough.

@sujameslin
Copy link
Contributor

@ianlin Can we merge this PR?
For me, it looks good.

@ianlin ianlin merged commit b85dc00 into react-native-webrtc:master Mar 5, 2018
@ianlin
Copy link
Member

ianlin commented Mar 5, 2018

Thanks guys!

@aarkalyk aarkalyk deleted the contact-identifier-added branch July 7, 2018 11:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants