Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[CIS-891] Flawless message list scrolling #1219

Merged
merged 20 commits into from
Jun 28, 2021
Merged

[CIS-891] Flawless message list scrolling #1219

merged 20 commits into from
Jun 28, 2021

Conversation

evsaev
Copy link
Contributor

@evsaev evsaev commented Jun 25, 2021 •
edited
Loading

This PR fixes all knows message-list scrolling issues 🎉

@evsaev evsaev added the 🎨 SDK: StreamChatUI Tasks related to the StreamChatUI SDK label Jun 25, 2021
@evsaev evsaev requested review from tbarbugli and VojtaStavik June 25, 2021 16:58
@Stream-SDK-Bot
Copy link
Collaborator

Stream-SDK-Bot commented Jun 25, 2021 •
edited
Loading

1 Warning
⚠️ Big PR

Generated by 🚫 Danger

@evsaev evsaev force-pushed the CIS-891-flawless-message-list-scrolling branch from aa9d526 to 9d12505 Compare June 28, 2021 12:25
@codecov-commenter
Copy link

codecov-commenter commented Jun 28, 2021 •
edited
Loading

Codecov Report

Merging #1219 (b716a9a) into main (49ccd37) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1219   +/-   ##
=======================================
  Coverage   91.27%   91.27%           
=======================================
  Files         217      217           
  Lines        9292     9292           
=======================================
  Hits         8481     8481           
  Misses        811      811
Flag Coverage Δ
llc-tests 91.27% <ø> (ø)
llc-tests-ios12 91.27% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 49ccd37...b716a9a. Read the comment docs.

@evsaev evsaev force-pushed the CIS-891-flawless-message-list-scrolling branch from 9d12505 to 7158f32 Compare June 28, 2021 12:49
skorepak
skorepak reviewed Jun 28, 2021
View reviewed changes
Copy link
Contributor

@skorepak skorepak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few comments. Looks good overall!

Sources/StreamChatUI/ChatMessageList/ChatMessageListVC.swift Show resolved Hide resolved
Sources/StreamChatUI/ChatMessageList/ChatMessageListVC.swift Show resolved Hide resolved
Sources/StreamChatUI/ChatMessageList/ChatMessageListVC.swift Outdated Show resolved Hide resolved
Sources/StreamChatUI/CommonViews/BaseViews.swift Show resolved Hide resolved
Sources/StreamChatUI/ChatMessageList/ChatThreadVC.swift Outdated Show resolved Hide resolved
Sources/StreamChatUI/ChatMessageList/ChatThreadVC.swift Show resolved Hide resolved
Sources/StreamChatUI/ChatMessageList/ChatMessage/ChatMessageCollectionViewCell.swift Outdated Show resolved Hide resolved
Sources/StreamChatUI/ChatMessageList/ChatMessage/ChatMessageCollectionViewCell.swift Outdated Show resolved Hide resolved
Sources/StreamChatUI/ChatMessageList/ChatMessageListCollectionView.swift Outdated Show resolved Hide resolved
Sources/StreamChatUI/ChatMessageList/ChatMessageListCollectionView.swift Outdated Show resolved Hide resolved
@evsaev evsaev force-pushed the CIS-891-flawless-message-list-scrolling branch 2 times, most recently from d563f1b to d613b67 Compare June 28, 2021 13:45
VojtaStavik
VojtaStavik reviewed Jun 28, 2021
View reviewed changes
Sources/StreamChatUI/ChatMessageList/ChatMessage/ChatMessageTableViewCell.swift Outdated Show resolved Hide resolved
Sources/StreamChatUI/ChatMessageList/ChatMessageListKeyboardObserver.swift Outdated Show resolved Hide resolved
Sources/StreamChatUI/ChatMessageList/ChatThreadVC.swift Show resolved Hide resolved
Sources/StreamChatUI/ChatMessageList/ChatThreadVC.swift
Comment on lines 403 to 405
open func messageContentViewDidTapOnThread(_ indexPath: IndexPath?) {
guard let indexPath = indexPath else { return log.error("IndexPath is not available") }
guard let channel = channelController.channel else { return }

let controller = _ChatThreadVC<ExtraData>()
controller.channelController = channelController
controller.messageController = channelController.client.messageController(
cid: channel.cid,
messageId: messageController.replies[indexPath.item].id
)
navigationController?.show(controller, sender: self)
log.error("Nestead threads are not supported")
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is not supported why do we have the method here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't provide empty default implementations for ChatMessageContentViewDelegate. should we?

Sources/StreamChatUI/ChatMessageList/Reactions/ChatMessageReactionsVC.swift Show resolved Hide resolved
Sources/StreamChatUI/ChatMessageList/ChatMessageListVC.swift Outdated Show resolved Hide resolved
@evsaev evsaev force-pushed the CIS-891-flawless-message-list-scrolling branch from d613b67 to a6a4258 Compare June 28, 2021 15:38
@evsaev evsaev force-pushed the CIS-891-flawless-message-list-scrolling branch from a6a4258 to b716a9a Compare June 28, 2021 15:49
@evsaev evsaev marked this pull request as ready for review June 28, 2021 15:50
VojtaStavik
VojtaStavik approved these changes Jun 28, 2021
View reviewed changes
Copy link
Contributor

@VojtaStavik VojtaStavik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great 🎉 Just a few nit comments.

CHANGELOG.md Outdated Show resolved Hide resolved
Sources/StreamChatUI/ChatMessageList/ChatMessage/ChatMessageCell.swift Outdated Show resolved Hide resolved
Sources/StreamChatUI/ChatMessageList/ChatMessageListVC.swift Outdated Show resolved Hide resolved
Sources/StreamChatUI/ChatMessageList/ChatMessageListVC.swift Outdated Show resolved Hide resolved
Sources/StreamChatUI/ChatMessageList/ChatMessageListView.swift Show resolved Hide resolved
Sources/StreamChatUI/ChatMessageList/ChatThreadVC.swift Outdated Show resolved Hide resolved
@evsaev evsaev force-pushed the CIS-891-flawless-message-list-scrolling branch from b716a9a to 9a78805 Compare June 28, 2021 16:19
@evsaev evsaev force-pushed the CIS-891-flawless-message-list-scrolling branch from 9a78805 to 2fa6ece Compare June 28, 2021 16:23
@evsaev evsaev merged commit f4668db into main Jun 28, 2021
@evsaev evsaev deleted the CIS-891-flawless-message-list-scrolling branch June 28, 2021 17:14
nuno-vieira
nuno-vieira reviewed Jun 28, 2021
View reviewed changes
Sources/StreamChatUI/ChatMessageList/ChatMessage/ChatMessageCell.swift
Comment on lines +22 to +25
/// The minimum spacing below the cell.
public var minimumSpacingBelow: CGFloat = 2 {
didSet { updateBottomSpacing() }
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minimumBottomSpace is a bit more clear and conventional?

Sources/StreamChatUI/ChatMessageList/ChatMessageListKeyboardObserver.swift
Comment on lines -49 to -50
let convertedKeyboardFrame = containerView.convert(keyboardFrame, from: UIScreen.main.coordinateSpace)
let intersectedKeyboardHeight = containerView.frame.intersection(convertedKeyboardFrame).height
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deleting these lines will make the keyboard observer not supporting undocked keyboards on the iPad: More information in this WWDC talk: https://developer.apple.com/videos/play/wwdc2017/242/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for finding this 👍 put back in #1229

Sources/StreamChatUI/ChatMessageList/ChatMessageListView.swift
Comment on lines +12 to +13
/// - listView: A view requesting date
/// - indexPath: An index path that should be used to get the date
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting data? Instead of date? In both arguments docs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the scroll overlay shows the date for the message it intersects with that's from where the naming came

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nuno-vieira nuno-vieira nuno-vieira left review comments

@skorepak skorepak skorepak left review comments

@VojtaStavik VojtaStavik VojtaStavik approved these changes

@tbarbugli tbarbugli Awaiting requested review from tbarbugli

Assignees

@VojtaStavik VojtaStavik

Labels
🎨 SDK: StreamChatUI Tasks related to the StreamChatUI SDK
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@evsaev @Stream-SDK-Bot @codecov-commenter @VojtaStavik @skorepak @nuno-vieira