Conversation
|
Once you get closer to having it ready, please also share a video of the ui - thank you 👍 |
|
@tomholub here is video of current implementation of Simulator.Screen.Recording.-.iPhone.13.-.2022-02-16.at.11.41.32.mp4 |
|
Looks good so far 👍 |
# Conflicts: # Gemfile.lock
# Conflicts: # appium/tests/screenobjects/new-message.screen.ts
tomholub
left a comment
tomholub
left a comment
There was a problem hiding this comment.
Code looks good. I'll test run it now
| private func updateRecipientsNode(layoutHeight: CGFloat, type: RecipientType) { | ||
| let currentHeight = self.recipientsNodeHeight(type: type) | ||
|
|
||
| guard currentHeight != layoutHeight, layoutHeight > 0 else { | ||
| return | ||
| } | ||
|
|
||
| switch type { | ||
| case .to: | ||
| self.calculatedRecipientsToPartHeight = layoutHeight | ||
| case .cc: | ||
| self.calculatedRecipientsCcPartHeight = layoutHeight | ||
| case .bcc: | ||
| self.calculatedRecipientsBccPartHeight = layoutHeight | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
There's quite a bit of rendering or layouting code now in this VC, which makes business logic less obvious. Would it make sense to move it into some sort of decorator or rendering file (specific for this VC) instead of being here? Could be another PR, more of a maintenance task.
tomholub
left a comment
tomholub
left a comment
There was a problem hiding this comment.
I'll approve and merge this but it needs improvement.
First: Cc/Bcc recipients are not respected on reply-all. To reproduce:
- open a message that has cc or bcc recipients in it. In my case, it was
to:me,cc:many people. It could also befrom:me,to:somebody,cc:others,bcc:the restfor example. - tap reply all
- the message I'm composing now is
to:original senderbut it's missing all the cc.
Second: The UI is disorderly. Let's follow what Gmail app does more closely - they do it well:
- instead of
Add Recipientplaceholder, let's putTolabel to the left of the input that stays there - Similarly for cc and bcc when we expand it. Add
ccandbccto the left as labels - Let's auto-collapse all of this, only showing the email bubbles, when switches to subject or body (lost focus)
- Let's re-expand the recipient area when user taps on the collapsed rendering
This way, we no longer need to have Add Recipient, Add cc Recipient, Add bcc recipient placeholders which take up a lot of space, each occupying own line. When user adds a few recipients, it quickly overwhelms my small screen (12 mini) whereas on Gmail app, it's comfortable.
2 participants
This PR adds cc and bcc recipients on compose screen
close #954
close #1352
Tests (delete all except exactly one):
CheckComposeEmailAfterReopeningtestTo be filled by reviewers
I have reviewed that this PR... (tick whichever items you personally focused on during this review):