Conversation
tomholub
left a comment
tomholub
left a comment
There was a problem hiding this comment.
code looks good 👍 see comment below. Not required but will come handy.
|
Functionality works well but I find the arrows confusing (didn't think about that before). Could you please, instead of the forward button, add three dots that pulls up a menu - where forward is one of the options (like Gmail app does it). |
|
Then may be we should show 3 dots instead of |
tomholub
left a comment
tomholub
left a comment
There was a problem hiding this comment.
Code and test looks great, except the UX concern above.
| layout.minimumLineSpacing = Constants.minimumLineSpacing | ||
| layout.sectionInset = Constants.sectionInset | ||
| let collectionNode = ASCollectionNode(collectionViewLayout: layout) | ||
| collectionNode.accessibilityIdentifier = "recipientsList" |
I'd like to follow Gmail in functionality since users will be migrating from Gmail app to here. Gmail doesn't have a chevron, true. But it does allow to expand/collapse by tapping the message header. Here's a related issue: #952 I guess, what you proposed brings us closer there. I just don't know if it's unnecessary work, just for the newly added "collapse" menu option to be removed again in #952 ? |
|
It seems like we already support opening message by tapping anywhere. So it's just about collapsing. Just by adding the collapse functionality when tapping expanded message header, and then adding a horizontal line between messages in thread, you would have also fixed #952 at the same time. Does it make sense in one PR? Or should I merge this PR and assign that one to you? |
|
Let's do it in 1 PR, so forwarding functionality will have final UX before merging. |
|
This also becomes quite a duplicate of #723 which I'll close now. |
| let count = input[section-1].processedMessage?.attachments.count ?? 0 | ||
| return Parts.allCases.count + count | ||
| let attachmentsCount = input[section-1].processedMessage?.attachments.count ?? 0 | ||
| return Parts.allCases.count + attachmentsCount |
tomholub
left a comment
tomholub
left a comment
There was a problem hiding this comment.
Functionality works well. One improvement: when forwarding (unlike reply) the recipients field should be in automatic focus when the compose view renders. When replying, the focus should be on message body as it does now.
2 participants
This PR adds forwarding functionality
close #900
close #952
Tests (delete all except exactly one):
To be filled by reviewers
I have reviewed that this PR... (tick whichever items you personally focused on during this review):