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.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

keepContentOffsetAtBottomOnBatchUpdates add tolerance range #9

Closed
mschonvogel opened this issue Feb 2, 2021 · 19 comments
Closed

keepContentOffsetAtBottomOnBatchUpdates add tolerance range #9

mschonvogel opened this issue Feb 2, 2021 · 19 comments
Labels
enhancement New feature or request

Comments

@mschonvogel
Copy link

Hello @ekazaev 馃憢 Currently, the layout only scrolls automatically when you have scrolled all the way down. Can you add a configuration for a tolerance range? So that even if you are for example 100pt from the bottom of the Scrollview and a new message comes in, it still scrolls automatically?

@ekazaev
Copy link
Owner

ekazaev commented Feb 3, 2021

@mschonvogel hi

I was thinking about it but I think iMessage behaves differently. It seems to be controlled by the host during the sending or receiving the new message. Can you please describe the behavior with that tolerance?

@ekazaev ekazaev added the enhancement New feature or request label Feb 3, 2021
@ekazaev
Copy link
Owner

ekazaev commented Feb 3, 2021

@mschonvogel
Copy link
Author

@ekazaev The left screen should automatically scroll to the latest message here
https://user-images.githubusercontent.com/370839/106735887-105feb00-6615-11eb-9c70-02f8fdd3a1de.mov

@ekazaev
Copy link
Owner

ekazaev commented Feb 3, 2021

Or do you mean that it should scroll fully to the bottom if the new message arrived?
It seems to me that it should be decided and implemented in the host app. For collection view layout it just a cell. IT does not distinguish them from each other. It can be a new message, it can be "Delivered/Read" bage change, typing indicator. So I think that if "new message comes" - the app knows that and can scroll to the new message if user is within some amount of pixels from the bottom.

@ekazaev
Copy link
Owner

ekazaev commented Feb 3, 2021

@mschonvogel I see. Thanks. It seems to me like its an app responsibility tbh. And it seems that it is kinda easy to implement?
You dont expect that collection will scroll to the bottom every time a typing indicator appears right? It is important only for the actual messages.

@mschonvogel
Copy link
Author

Yes, I can implement that in the host app of course. But I thought that somehow makes sense since the layout already takes care of setting the content offset automatically

@mschonvogel
Copy link
Author

mschonvogel commented Feb 3, 2021

I expect the collectionview to scroll if I'm like max 20points away from the bottom @ekazaev

@ekazaev
Copy link
Owner

ekazaev commented Feb 3, 2021

It just sounds like it has logic inside. If new message -> Scroll, If typing indicator appears -> Dont, If we inserted "read" cell -> dont scroll.

I understand you, but it is something that should be implemented in the host app. Or a framework built around ChatLayout. But not a layout itself.

@mschonvogel
Copy link
Author

Okay, all right, I'll implement this myself 馃檪

@ekazaev
Copy link
Owner

ekazaev commented Feb 3, 2021

Sorry about that.
I just see that if it will be in the layout you will have to implement the opposite logic in the host app:
If new message -> Set tolerance to 20px, If typing indicator appears -> Set tolerance to 0, If we inserted "read" cell -> Set tolerance to 0.

@mschonvogel
Copy link
Author

I understand what you mean but I still disagree, which is not a problem 馃檪. The case I want to cover here is: The user thinks they are at the end of the chat history, but they're not, because 10pt away. So I would expect scrolling for all inserts. typing indicator, state update etc.

@ekazaev
Copy link
Owner

ekazaev commented Feb 3, 2021

I am just trying to find the right balance here :)
I use that layout in my work. And I remember (even though that was in the view controller) that I was scrolling down every time something appeared at the bottom. And it did not pas a QA because I got "Dont scroll user to the bottom every time a typing indicator appears". And in my work app - the "delivered"/"read" indicator is not a part of the cell like in example app, but a separate cell under it. Same applied to it.

@mschonvogel
Copy link
Author

Yes, I understand the thinking of your QA department, but in that case, if you are only a few pixels off the bottom, I would still scroll automatically 鈽濓笍馃榾

@ekazaev
Copy link
Owner

ekazaev commented Feb 3, 2021

Something like this will solve the issue in the example 馃槀:
But we start to bring questions like "Should we scroll if the user is dragging the collection?" "Shall we scroll before the new message appears or after". I have the feeling that it is something that only hosting app can answer

                                  completion: { _ in
                                      if self.collectionView.contentOffset.y > self.collectionView.contentSize.height - self.collectionView.frame.height + self.collectionView.adjustedContentInset.bottom - tolerance {
                                          self.collectionView.collectionViewLayout.invalidateLayout()
                                          self.scrollToBottom(completion: completion)
                                      } else {
                                          completion?()
                                      }
                                  },

@ekazaev
Copy link
Owner

ekazaev commented Feb 3, 2021

Let me sleep with that thought. @mschonvogel
I mean, I understand where are you coming from. The question is if keepContentOffsetAtBottomOnBatchUpdates is not enabled, but tolerance is set. Should it scroll to the top within that tolerance?

@ekazaev
Copy link
Owner

ekazaev commented Feb 3, 2021

test.mov

@mschonvogel
Copy link
Author

That looks good! I'd probably utilize an enum for that, maybe like so:

enum KeepContentOffsetAtBottomBehavior {
  case disabled
  case enabled(tolerance: CGFloat)
}

@ekazaev
Copy link
Owner

ekazaev commented Feb 3, 2021

Ill think about it. I promise. But for now everything that comes into my head tells me against it.

@ekazaev
Copy link
Owner

ekazaev commented Feb 3, 2021

I think it actually has sense to scroll to the bottom before anything appears if user within that tolerance to the bottom.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants