Add didTapAccessoryView Delegate method and adjust layout by nathantannar4 · Pull Request #834 · MessageKit/MessageKit

nathantannar4 · 2018-08-31T18:05:59Z

No description provided.

nathantannar4 · 2018-08-31T18:06:23Z

Solves #770

SD10 · 2018-08-31T18:40:41Z

Example/Sources/View Controllers/AdvancedExampleViewController.swift

+    func configureAccessoryView(_ accessoryView: UIView, for message: MessageType, at indexPath: IndexPath, in messagesCollectionView: MessagesCollectionView) {
+        let button = UIButton(type: .infoLight)
+        button.tintColor = .primaryColor
+        accessoryView.addSubview(button)


This subview will have to be removed somewhere because cells are reused?

Added with a comment to remind devs of this

SD10 · 2018-08-31T18:41:49Z

Sources/Views/Cells/MessageContentCell.swift

+
+        // Accessory view aligned to the middle of the messageContainerView
+        var y = (messageContainerView.bounds.height - attributes.accessoryViewSize.height) / 2
+        y += messageContainerView.frame.origin.y


Pretty sure the cellContentHeight method will be incorrect due to this new alignment

Maybe not actually 🤔 Need to review in more depth

@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

@SD10 Made this change

SD10

@nathantannar4 Really appreciate you stepping up and taking this on 🥇 💯 💪 Just a few things

nathantannar4 · 2018-08-31T19:20:27Z

@SD10 💪

SD10 · 2018-08-31T19:36:44Z

Sources/Layout/MessageSizeCalculator.swift

        let accessoryViewVerticalPadding = accessoryViewPadding(for: message).vertical
-        let accessoryViewTotalHeight = accessoryViewHeight + accessoryViewVerticalPadding
+        let accessoryViewYOriginInset = cellTopLabelHeight + messageTopLabelHeight + messageVerticalPadding + messageContainerPadding(for: message).top
+        let accessoryViewTotalHeight = accessoryViewHeight + accessoryViewVerticalPadding + accessoryViewYOriginInset


Care to explain what you're doing here? I'll review later but the logic was already super confusing before this 🤔

Adding the extra distance that the accessoryView's origin starts at

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 🤔

Idk if this is a better way. I was just trying to come to a solution. I can adjust

SD10 · 2018-09-01T03:38:14Z

Sources/Layout/MessageSizeCalculator.swift

        let accessoryViewVerticalPadding = accessoryViewPadding(for: message).vertical
-        let accessoryViewTotalHeight = accessoryViewHeight + accessoryViewVerticalPadding
+        let accessoryViewYOriginInset = cellTopLabelHeight + messageTopLabelHeight + messageContainerPadding(for: message).top
+        let accessoryViewTotalHeight = accessoryViewHeight + accessoryViewVerticalPadding + accessoryViewYOriginInset


This should just be:

let accessoryViewTotalHeight = accessoryViewHeight + accessoryViewVerticalPadding

No need for accessoryViewYOriginInset, see the reason why below

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

SD10 · 2018-09-11T01:29:36Z

Alright, fixing this now 😓

nathantannar4 · 2018-09-26T06:05:45Z

@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

SD10 · 2018-09-26T06:31:49Z

@nathantannar4 Thanks for taking lead here 👍 The problem here is we shouldn't ship an API where the accessory view has a UIEdgeInsets for padding because they should not be able to set any vertical padding -- only left and right. As it is, the current layout logic for the accessory view's Y position doesn't make any sense

SD10 · 2018-09-26T06:34:07Z

Sources/Views/Cells/MessageContentCell.swift

        // 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)


This part is perfect

SD10 · 2018-09-26T06:35:53Z

Sources/Views/Cells/MessageContentCell.swift

-        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


This code isn't doing anything except ruining the calculation above. This is what I mean by no vertical padding

SD10 · 2018-09-26T06:38:59Z

Example/Sources/View Controllers/AdvancedExampleViewController.swift

        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))


An unfortunate API at this point:

incomingAccessoryViewLeftPadding incomingAccessoryViewRightPadding outgoingAccessoryViewLeftPadding outgoingAccessoryViewRightPadding

Accessoryview updates try to fix

…updates

nathantannar4 · 2018-10-12T05:23:16Z

Current state of this PR:

Under fair sizing of the accessoryView, ex: 30x30 the layout looks as follows
Under sizing that breaks layout, ie: developer error

SD10 · 2018-10-12T21:07:42Z

Sources/Layout/MessageSizeCalculator.swift

                + messageContainerHeight + messageVerticalPadding + messageBottomLabelHeight
            let cellHeight = max(avatarHeight, totalLabelHeight)
-            return max(accessoryViewTotalHeight, cellHeight)
+            return cellHeight


Are we sure this was the correct change? 🤔 Seems like it was correct before

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

SD10 · 2018-10-12T21:10:03Z

Sources/Layout/MessageSizeCalculator.swift

        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


These should be subtracted, not added?

Reverted, I goofed

Sources/Models/HorizontalEdgeInsets.swift

Add delegate and adjust layout

a3c7a2f

nathantannar4 self-assigned this Aug 31, 2018

nathantannar4 requested review from SD10 and zhongwuzw August 31, 2018 18:06

CHANGELOG Entry

041a239

nathantannar4 added this to the 1.1 milestone Aug 31, 2018

SD10 reviewed Aug 31, 2018

View reviewed changes

SD10 added the under review label Aug 31, 2018

SD10 reviewed Aug 31, 2018

View reviewed changes

SD10 reviewed Sep 1, 2018

View reviewed changes

nathantannar4 force-pushed the accessoryview-updates branch from 0f93660 to 041a239 Compare September 1, 2018 19:31

Minor edits

406f700

SD10 reviewed Sep 26, 2018

View reviewed changes

[Accessoryview] Fix

bc70b5b

JulienKode mentioned this pull request Oct 4, 2018

[Accessoryview] fix #893

Merged

nathantannar4 added 3 commits October 11, 2018 21:30

Merge pull request #893 from JulienKode/accessoryview-updates-fix

83b483f

Accessoryview updates try to fix

Merge remote-tracking branch 'origin/development' into accessoryview-…

78c48fb

…updates

Horizontal Padding only for accessoryView

2e6de00

SD10 reviewed Oct 12, 2018

View reviewed changes

Sources/Models/HorizontalEdgeInsets.swift Show resolved Hide resolved

Final tweak to layout

197ec8b

nathantannar4 merged commit 50bce2a into development Oct 13, 2018

hyouuu deleted the accessoryview-updates branch February 17, 2020 07:09

Conversation

nathantannar4 commented Aug 31, 2018

Uh oh!

nathantannar4 commented Aug 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nathantannar4 Sep 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SD10 Aug 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SD10 Sep 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SD10 left a comment

Choose a reason for hiding this comment

Uh oh!

nathantannar4 commented Aug 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SD10 Aug 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SD10 Sep 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SD10 commented Sep 11, 2018

Uh oh!

nathantannar4 commented Sep 26, 2018

Uh oh!

SD10 commented Sep 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nathantannar4 commented Oct 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nathantannar4 Sep 26, 2018 •

edited

Loading

SD10 Aug 31, 2018 •

edited

Loading

SD10 Sep 1, 2018 •

edited

Loading

SD10 Aug 31, 2018 •

edited

Loading

SD10 Sep 1, 2018 •

edited

Loading