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
Add didTapAccessoryView Delegate method and adjust layout #834
Conversation
Solves #770 |
func configureAccessoryView(_ accessoryView: UIView, for message: MessageType, at indexPath: IndexPath, in messagesCollectionView: MessagesCollectionView) { | ||
let button = UIButton(type: .infoLight) | ||
button.tintColor = .primaryColor | ||
accessoryView.addSubview(button) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This subview will have to be removed somewhere because cells are reused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added with a comment to remind devs of this
|
||
// Accessory view aligned to the middle of the messageContainerView | ||
var y = (messageContainerView.bounds.height - attributes.accessoryViewSize.height) / 2 | ||
y += messageContainerView.frame.origin.y |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure the cellContentHeight
method will be incorrect due to this new alignment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe not actually 🤔 Need to review in more depth
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nathantannar4 I think your logic for this is:
var y = messageContainerView.frame.midY - (attributes.accessoryViewSize.height/2)
This is a tough area of the codebase I know
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SD10 Made this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nathantannar4 Really appreciate you stepping up and taking this on 🥇 💯 💪 Just a few things
@SD10 💪 |
@@ -97,7 +97,8 @@ open class MessageSizeCalculator: CellSizeCalculator { | |||
let avatarVerticalPosition = avatarPosition(for: message).vertical | |||
let accessoryViewHeight = accessoryViewSize(for: message).height | |||
let accessoryViewVerticalPadding = accessoryViewPadding(for: message).vertical | |||
let accessoryViewTotalHeight = accessoryViewHeight + accessoryViewVerticalPadding | |||
let accessoryViewYOriginInset = cellTopLabelHeight + messageTopLabelHeight + messageVerticalPadding + messageContainerPadding(for: message).top | |||
let accessoryViewTotalHeight = accessoryViewHeight + accessoryViewVerticalPadding + accessoryViewYOriginInset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Care to explain what you're doing here? I'll review later but the logic was already super confusing before this 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding the extra distance that the accessoryView's origin starts at
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
messageVerticalPadding
already includes messageContainerPadding(for: message).top
. Not sure if I really understand this approach? Shouldn't the logic be the same for resolving the height against the AvatarView
when it is positioned .messageCenter
?
If you think you have a better way of calculating it -- I'm open to suggestions -- but I can't understand what trick you're using so I'm skeptical 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idk if this is a better way. I was just trying to come to a solution. I can adjust
@@ -97,7 +97,8 @@ open class MessageSizeCalculator: CellSizeCalculator { | |||
let avatarVerticalPosition = avatarPosition(for: message).vertical | |||
let accessoryViewHeight = accessoryViewSize(for: message).height | |||
let accessoryViewVerticalPadding = accessoryViewPadding(for: message).vertical | |||
let accessoryViewTotalHeight = accessoryViewHeight + accessoryViewVerticalPadding | |||
let accessoryViewYOriginInset = cellTopLabelHeight + messageTopLabelHeight + messageContainerPadding(for: message).top | |||
let accessoryViewTotalHeight = accessoryViewHeight + accessoryViewVerticalPadding + accessoryViewYOriginInset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should just be:
let accessoryViewTotalHeight = accessoryViewHeight + accessoryViewVerticalPadding
No need for accessoryViewYOriginInset
, see the reason why below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason is because the cellContentHeight
logic was already fine as is 😆. Do you mind cleaning up the git history by squashing these commits? Since we flip flopped back and forth
EDIT: I think I may have to make a slight change actually... jeez
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted
0f93660
to
041a239
Compare
Alright, fixing this now 😓 |
@SD10 I think this is fairly solid in its current state. Layout gets messed up when absurdly large accessory view sizes are set, but that could be listed as a known bug so we can push the 1.1 release which will fix the bottom inset issues. Then we could update to Swift 4.2 for a 1.2 update |
@nathantannar4 Thanks for taking lead here 👍 The problem here is we shouldn't ship an API where the accessory view has a |
@@ -275,8 +275,7 @@ open class MessageContentCell: MessageCollectionViewCell { | |||
open func layoutAccessoryView(with attributes: MessagesCollectionViewLayoutAttributes) { | |||
|
|||
// Accessory view aligned to the middle of the messageContainerView | |||
var y = (messageContainerView.bounds.height - attributes.accessoryViewSize.height) / 2 | |||
y += messageContainerView.frame.origin.y | |||
var y = messageContainerView.frame.midY - (attributes.accessoryViewSize.height/2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part is perfect
@@ -275,8 +275,7 @@ open class MessageContentCell: MessageCollectionViewCell { | |||
open func layoutAccessoryView(with attributes: MessagesCollectionViewLayoutAttributes) { | |||
|
|||
// Accessory view aligned to the middle of the messageContainerView | |||
var y = (messageContainerView.bounds.height - attributes.accessoryViewSize.height) / 2 | |||
y += messageContainerView.frame.origin.y | |||
var y = messageContainerView.frame.midY - (attributes.accessoryViewSize.height/2) | |||
y -= attributes.accessoryViewPadding.vertical + attributes.accessoryViewPadding.top |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code isn't doing anything except ruining the calculation above. This is what I mean by no vertical padding
@@ -97,6 +97,11 @@ final class AdvancedExampleViewController: ChatViewController { | |||
layout?.setMessageIncomingAvatarSize(CGSize(width: 30, height: 30)) | |||
layout?.setMessageIncomingMessagePadding(UIEdgeInsets(top: -outgoingAvatarOverlap, left: -18, bottom: outgoingAvatarOverlap, right: 18)) | |||
|
|||
layout?.setMessageIncomingAccessoryViewSize(CGSize(width: 30, height: 30)) | |||
layout?.setMessageIncomingAccessoryViewPadding(UIEdgeInsets(top: 0, left: 8, bottom: 0, right: 0)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An unfortunate API at this point:
incomingAccessoryViewLeftPadding
incomingAccessoryViewRightPadding
outgoingAccessoryViewLeftPadding
outgoingAccessoryViewRightPadding
Accessoryview updates try to fix
|
||
switch avatarVerticalPosition { | ||
case .messageCenter: | ||
let totalLabelHeight: CGFloat = cellTopLabelHeight + messageTopLabelHeight | ||
+ messageContainerHeight + messageVerticalPadding + messageBottomLabelHeight | ||
let cellHeight = max(avatarHeight, totalLabelHeight) | ||
return max(accessoryViewTotalHeight, cellHeight) | ||
return cellHeight |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure this was the correct change? 🤔 Seems like it was correct before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I thought this was one of the things you wanted reverted and if it messed up the height it was a developer error, back to how I originally had it :D
@@ -231,7 +228,7 @@ open class MessageSizeCalculator: CellSizeCalculator { | |||
let messagePadding = messageContainerPadding(for: message) | |||
let accessoryWidth = accessoryViewSize(for: message).width | |||
let accessoryPadding = accessoryViewPadding(for: message) | |||
return messagesLayout.itemWidth - avatarWidth - messagePadding.horizontal - accessoryWidth - accessoryPadding.horizontal | |||
return messagesLayout.itemWidth - avatarWidth - messagePadding.horizontal + accessoryWidth + accessoryPadding.horizontal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be subtracted, not added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted, I goofed
No description provided.