Skip to content

Added convenience method to set properties for all MessageSizeCalcula…#697

Merged
SD10 merged 3 commits intoMessageKit:developmentfrom
zhongwuzw:layout
May 23, 2018
Merged

Added convenience method to set properties for all MessageSizeCalcula…#697
SD10 merged 3 commits intoMessageKit:developmentfrom
zhongwuzw:layout

Conversation

@zhongwuzw
Copy link
Member

Support for #694 .

@zhongwuzw zhongwuzw changed the base branch from master to development May 22, 2018 10:21
@zhongwuzw
Copy link
Member Author

@SD10 Let's provide one shot method.

return calculator.sizeForItem(at: indexPath)
}

/** Set `incomingAvatarSize` of all `MessageSizeCalculator`s */
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit, but what about a triple / (///) instead of these multiline comments?

@ndonald2
Copy link

Looks good to me! Thanks for doing this.

}

/// Set `incomingAvatarSize` of all `MessageSizeCalculator`s
open func setMessageIncomingAvatarSize(_ newSize: CGSize) {
Copy link
Member

Choose a reason for hiding this comment

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

Think these should be public and not open? They're helper methods. I don't know if we should allow people to override? I don't see any major harm in leaving it but what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

No big differences I think, I'll change this to public.

@SD10
Copy link
Member

SD10 commented May 22, 2018

🙏 I'm so lucky to have other contributors 😭

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.

Great 💯

@SD10 SD10 merged commit 818a881 into MessageKit:development May 23, 2018
@zhongwuzw zhongwuzw deleted the layout branch May 23, 2018 02:58
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.

4 participants

Comments