Skip to content

Fix scrollToBottom && remove scrollsToBottomOnFirstLayout#395

Merged
SD10 merged 6 commits intoMessageKit:developmentfrom
zhongwuzw:fix-scroll-to-bottm
Dec 12, 2017
Merged

Fix scrollToBottom && remove scrollsToBottomOnFirstLayout#395
SD10 merged 6 commits intoMessageKit:developmentfrom
zhongwuzw:fix-scroll-to-bottm

Conversation

@zhongwuzw
Copy link
Copy Markdown
Member

No description provided.

@zhongwuzw zhongwuzw requested a review from SD10 December 11, 2017 07:23
This was referenced Dec 11, 2017
scrollToItem(at: indexPath, at: .bottom, animated: animated)

let collectionViewContentHeight = collectionViewLayout.collectionViewContentSize.height
let isContentTooSmall = (collectionViewContentHeight < bounds.height * 2)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why bounds.height * 2 here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The reason is scrollToItem would sometimes invalid when collectionViewContentSize's height slightly larger than collectionView's height.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, I understand that, but why did we decide to use * 2? Is this just a random number?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Nope, it's based on my tests, I found scrollToItem sometimes invalid when last cell's maxY greater than bounds's height but less than 2 * bounds's height.

@SD10
Copy link
Copy Markdown
Member

SD10 commented Dec 11, 2017

@zhongwuzw This looks good. I fixed the problem with the MessageLabel font.

The only thing I notice about this change is there is a small flicker from using batch updates 🤔

@SD10
Copy link
Copy Markdown
Member

SD10 commented Dec 11, 2017

@zhongwuzw I also want to mention you don't have to work from a fork of the project, you're a contributor so you can work from branches on the main MessageKit repo 😅

@zhongwuzw
Copy link
Copy Markdown
Member Author

@SD10

The only thing I notice about this change is there is a small flicker from using batch updates
I think we can use scrollToBottom(animated: true), the flicker is because of not animated.

Copy link
Copy Markdown
Member

@SD10 SD10 left a comment

Choose a reason for hiding this comment

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

@zhongwuzw Can you add a CHANGELOG entry under Removed for the scrollsToBottomOnFirstLayout flag? And another under Fixed for scrollToBottom()?

@zhongwuzw
Copy link
Copy Markdown
Member Author

@SD10 , I added the changelog.

Copy link
Copy Markdown
Member

@SD10 SD10 left a comment

Choose a reason for hiding this comment

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

Awesome work @zhongwuzw 💯

@SD10 SD10 merged commit 769dc74 into MessageKit:development Dec 12, 2017
@SD10 SD10 removed the under review label Dec 12, 2017
@zhongwuzw zhongwuzw deleted the fix-scroll-to-bottm branch December 15, 2017 05:04
@SD10 SD10 mentioned this pull request Dec 18, 2017
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants