Skip to content

Enhancement/transit info detection#520

Merged
SD10 merged 5 commits intoMessageKit:developmentfrom
nosarj:enhancement/transitInfoDetection
Mar 4, 2018
Merged

Enhancement/transit info detection#520
SD10 merged 5 commits intoMessageKit:developmentfrom
nosarj:enhancement/transitInfoDetection

Conversation

@nosarj
Copy link
Contributor

@nosarj nosarj commented Feb 15, 2018

What does this implement/fix? Explain your changes.

This PR adds transit information detection option to message labels

Does this close any currently open issues?

Contributes but does not complete Data detectors for MessageLabel #71

Where has this been tested?

Devices/Simulators: iphone 8

iOS Version: … 11.2

Swift Version: …4

MessageKit Version: 0.13.1

Copy link
Member

Choose a reason for hiding this comment

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

Why did we change .addressComponents here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has been marked as "// Deprecated in favor of components" so I updated it

Copy link
Member

Choose a reason for hiding this comment

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

Looks good to me. I don't see this as deprecated in the Foundation docs. Just need to test that it works before merging 👍

Copy link
Member

Choose a reason for hiding this comment

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

The comments not documented in Apple API Reference, so please make sure this is available in iOS9 - iOS11.(IMO, maybe we can do this change after documented in API Reference 😄 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

happy to take it out if you prefer, but prob a good one to remember for when it does go

Copy link
Member

Choose a reason for hiding this comment

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

Maybe change the parameter name transitInformationComponents -> components

Copy link
Member

Choose a reason for hiding this comment

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

We could just use transitInfoComponents, shorter 😅

Copy link
Member

@SD10 SD10 left a comment

Choose a reason for hiding this comment

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

Hey @nosarj 👋,
Thanks for making this PR 😄 If you could address the requested changes this LGTM. We just need to add a CHANGELOG entry under Added for the new DetectorType and list this as a breaking change.

Copy link
Member

@SD10 SD10 left a comment

Choose a reason for hiding this comment

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

thanks @nosarj

@SD10 SD10 merged commit e00a8f4 into MessageKit:development Mar 4, 2018
@SD10
Copy link
Member

SD10 commented Mar 4, 2018

Thank you for contributing to MessageKit! I've invited you to join the MessageKit GitHub organization - no pressure to accept! If you'd like more information on what that means, check out our contributing guidelines and join the MessageKit Slack channel. Feel free to reach out if you have any questions! 😃

@SD10 SD10 removed the under review label Mar 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

Comments