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

Move tap gesture from cell to collectionView #417

Merged
merged 3 commits into from
Dec 21, 2017

Conversation

zhongwuzw
Copy link
Member

Reduce tap gesture amount by move tap gesture from cell to collectionView.

@zhongwuzw zhongwuzw requested a review from SD10 December 20, 2017 03:40
@SD10
Copy link
Member

SD10 commented Dec 20, 2017

@zhongwuzw Can you explain this a little more to me? It feels awkward moving this into MessagesCollectionView

Sent with GitHawk

lufzi added a commit to kaodim/MessageKit that referenced this pull request Dec 20, 2017
@nathantannar4
Copy link
Member

Would it help performance? Having one tap gesture rather than X. I know you can very easily translate a tap to a point and this get the cell that was tapped

Sent with GitHawk

@zhongwuzw
Copy link
Member Author

@SD10 @nathantannar4 , two reasons for changes.

  1. As @nathantannar4 says, it's for performance, we don't need to create tap gesture for each cell.
  2. At the moment, we only need tap gesture on messageContainerView of cell, maybe we need tap handler on headerViewFooterView..... in the future,we must add more and more tap gesture. But by translate the tap gesture to collectionView, we can easily do it.

@SD10
Copy link
Member

SD10 commented Dec 21, 2017

@zhongwuzw Can you add a CHANGELOG entry?

Changed

  • Breaking Change Moved the handleTapGesture(_ gesture: UIGestureRecognizer) method from MessagesCollectionViewCell to MessagesCollectionView.

@zhongwuzw zhongwuzw self-assigned this Dec 21, 2017
@zhongwuzw zhongwuzw merged commit 75ff27a into development Dec 21, 2017
@zhongwuzw zhongwuzw deleted the enhancement/message-cell-tap branch December 21, 2017 07:09
@zhongwuzw zhongwuzw mentioned this pull request Jan 17, 2018
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.

3 participants