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

Alternative solution for requiring direct taps for launching media vi… #1261

Merged
merged 1 commit into from
Jul 21, 2016

Conversation

gluxon
Copy link
Contributor

@gluxon gluxon commented Jul 19, 2016

First time contributor checklist

Contributor checklist

  • I'm following the code, UI and style conventions
  • My commits are rebased on the latest master branch
  • My commits are in nice logical chunks
  • My contribution is fully baked and is ready to be merged as is
  • I have tested my contribution on these devices:
    • iPad Pro 9.7", iOS 9.3.3
  • I have made the choice whether I want the BitHub reward or not by omitting or adding the word FREEBIE in my commit message

Description

Michael beat me to the solution for issue #1257. Although I wasn't able to get messageBubbleContainerView to properly instantiate, I think overriding the subclass's touch input method is a cleaner solution than the workaround in the message collection view controller. This is an alternative fix to the issue with images opening when clicking the white space horizontal to it, and allows info/error messages to be clickable.

I would not be offended if the other hack was used instead due to time constraints with the next Signal release. :)

@michaelkirk
Copy link
Contributor

I do like this approach. I think there are a lot of opportunities like this to move polymorphic logic into subclasses, simplifying our big hairy view controller classes. Thanks - will merge after testing against latest master.

@michaelkirk michaelkirk merged commit 583d3e8 into signalapp:master Jul 21, 2016
@michaelkirk
Copy link
Contributor

Thanks @gluxon! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants