fix: Removed duplicated alert system. - WPB-23037#4492
fix: Removed duplicated alert system. - WPB-23037#4492JaCaLlaSchwarz wants to merge 8 commits intodevelopfrom
Conversation
JaCaLlaSchwarz
changed the title
JaCaLlaSchwarz
changed the title
netbe
left a comment
netbe
left a comment
There was a problem hiding this comment.
Left a couple comments, it's going towards the right direction but still need some changes before approval.
I would expect conversation snapshot test to change too somehow
| if source == .slowSync { | ||
| // Slow synced conversations should be considered read from the start | ||
| conversation.lastReadServerTimeStamp = conversation.lastModifiedDate | ||
| } |
There was a problem hiding this comment.
question: this should not be removed or is it deadcode?
| // Slow synced conversations should be considered read from the start | ||
| localConversation.lastReadServerTimeStamp = localConversation.lastModifiedDate |
There was a problem hiding this comment.
issue: careful, I guess this is still needed
| connectionViewController = UserConnectionViewController(userSession: userSession, user: otherParticipant) | ||
| headerView = connectionViewController?.view | ||
| } else { | ||
| noUserConnectionViewController = NoUserConnectionViewController(userSession: userSession) |
There was a problem hiding this comment.
suggestion: I feel like the name of this view NoUserConnectionViewController is a bit misleading, as it's the header for groups or channels.
maybe DefaultConversationHeaderViewController, feel free to challenge this name ;)
|
|
||
| let connectionOrOneOnOne = conversation.conversationType == .connection || conversation | ||
| .conversationType == .oneOnOne | ||
| let connectionOrOneOnOne = [.connection, .oneOnOne /* , .group */ ] |
There was a problem hiding this comment.
suggestion: revert, this change is not necessary
| await context.perform { [self] in | ||
| if isInitialFetch { | ||
| // we just got a new conversation, we display new conversation header | ||
| localConversation.appendNewConversationSystemMessage( |
There was a problem hiding this comment.
issue: not inserting the new conversationsystemMessage is not enough (+ could be avoided). For current conversations the double system message would still show.
I suggest to not return a cell on the conversationContentViewController for the newConversation system message
Test Results 3 files 541 suites 3m 13s ⏱️ Results for commit 0071777. ♻️ This comment has been updated with latest results. Summary: workflow run #23638232142 |
JaCaLlaSchwarz
force-pushed
the
fix/remove_duplicated_warning_message_WPB23037_v2
branch
from
90c22a2 to
ae6308f
Compare
…ithub.com:wireapp/wire-ios into fix/remove_duplicated_warning_message_WPB23037_v2
|
|
FYI I'll wait for the issues @netbe raised to be addressed before review. |
3 participants
Issue
Duplicated Alert warning message
Testing
Checklist
[WPB-XXX].UI accessibility checklist
If your PR includes UI changes, please utilize this checklist: