Skip to content

MessageKit 1.0-beta#666

Merged
SD10 merged 137 commits intomasterfrom
development
May 3, 2018
Merged

MessageKit 1.0-beta#666
SD10 merged 137 commits intomasterfrom
development

Conversation

@SD10
Copy link
Member

@SD10 SD10 commented Apr 29, 2018

No description provided.

SD10 and others added 30 commits January 28, 2018 13:03
Open up MessagesCollectionViewFlowLayout cell sizing API
Document MediQuo application as a user of MessageKit
Move delegate methods to allow override
@hyouuu
Copy link
Contributor

hyouuu commented Apr 29, 2018

Finally!!!

@zhongwuzw
Copy link
Member

@SD10 , I prepare to remove Cocoapods dependency for Example project, I don't want to pod install every time when the framework changed, I want to just open Example project and click run button, what do you think? 😂

@SD10
Copy link
Member Author

SD10 commented Apr 30, 2018

@zhongwuzw I remember we changed to this setup because we want to make sure everything is working correctly on CocoaPods


open func numberOfSections(in collectionView: UICollectionView) -> Int {
guard let collectionView = collectionView as? MessagesCollectionView else {
fatalError(MessageKitError.notMessagesCollectionView)
Copy link
Member

Choose a reason for hiding this comment

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

TBO, we have much these fatalError, maybe we can have a better friendly way to do this (maybe assert or just use some default values)? other than just stop execution in Debug or Release.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, but I haven't found any good solution TBH. I had some default values before and removed them all because it just doesn't make sense 😕 We would be writing code that never gets executed. You have to set all 3 protocols MessagesDataSource, MessagesDisplayDelegate, and MessagesLayoutDelegate or nothing works

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think the fatalErrors here are valid. With good documentation in the superclasses this should not be an issue. And if it is; its a valid reason to crash there as there is no way to recover.

Changing these to asserts could be an option to prevent crashes in release builds, but I am not sure if that’s the best approach. How does it work with APIs like UITableView and UICollectionView?

Copy link
Member Author

@SD10 SD10 Apr 30, 2018

Choose a reason for hiding this comment

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

Thanks for the feedback @BasThomas!

How does it work with APIs like UITableView and UICollectionView?

This is not standard UITableView or UICollectionView behavior. Instead of extending the UIKit protocols, we conform to them internally and then call our own MessageKit protocols inside of them. This is to decouple the MessagesDataSource from UICollectionViewDataSource, so the project can be used with things like IGListKit. We're forced to provide a valid implementation or it is a fatalError 😕

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like a good usage of fatalError then. :)


// MARK: - Top cell Label

public func cellTopLabelSize(for message: MessageType, at indexPath: IndexPath) -> CGSize {
Copy link
Member

Choose a reason for hiding this comment

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

@SD10 Can we integrate these methods to one? it's a little bit redundancy ,cellTopLabel/messageTopLabel/messageBottomLabel's size and alignment are almost the same.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so 😕 I didn't even want these to be public but people have requested them to be accessible when they subclass. The only way to combine it into 1 method is to pass it an enum I think? I really don't like those kinds of API

Copy link
Member

Choose a reason for hiding this comment

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

👌

@SD10
Copy link
Member Author

SD10 commented Apr 30, 2018

Don't know if I want to merge this as is or ship the beta from development. I guess we can do the latter for future releases

@zhongwuzw
Copy link
Member

🤔 I prefer to choose just merge to master, if we ship the beta in development, how can we get the feedback from the user? 😂

@SD10
Copy link
Member Author

SD10 commented Apr 30, 2018

ah @zhongwuzw. I meant I can push a new release to CocoaPods from the development branch. That's what we do for Moya. I think this release is so large that you're probably right, master is better

zhongwuzw and others added 2 commits May 1, 2018 00:30
* Fixed separator line height

* Update CHANGELOG.md
@SD10
Copy link
Member Author

SD10 commented May 3, 2018

Probs gonna merge this if CI passes and open an issue for adding better docs or testing as starter tasks

@SD10
Copy link
Member Author

SD10 commented May 3, 2018

I wish I had the free time to benchmark this vs the current master. I'll probably come back to it when I do. I know I reduced layout time by 70% with #519 and some amazing changes by @zhongwuzw probably reduced it by half after that

@SD10 SD10 merged commit 69c5414 into master May 3, 2018
@SD10
Copy link
Member Author

SD10 commented May 3, 2018

Yeah, that just happened :shipit:

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.