Remove unused pageSize from enableLoadMore#196
Open
matthewrfennell wants to merge 1 commit intoexyte:mainfrom
Open
Remove unused pageSize from enableLoadMore#196matthewrfennell wants to merge 1 commit intoexyte:mainfrom
matthewrfennell wants to merge 1 commit intoexyte:mainfrom
Conversation
Pagination offset (the distance from the top message before we start
loading more) used to be determined by the offset parameter passed to
enableLoadMore, like so:
if ids.prefix(offset + 1).contains(message.id) {
onEvent?(message)
}
This got changed in 568d00d. Now, we
rely on paginationTargetIndexPath instead, which is hardcoded to be the
last already loaded message:
IndexPath(row: lastSection.rows.count - 1, section: sections.count - 1)
As a result, pageSize is no longer used. Remove it to make sure that
consumers do not expect different behaviour when they modify the
parameter.
71871bf to
0847045
Compare
Collaborator
|
Hey @matthewrfennell, this comment just means that we should store the target index, and can't rely on easy math on the fly, because of some swiftUI shenanigans. I think it's better to restore trigger offset functionality if at all possible, it will help preserve the API if nothing else. If you're willing to work on that, please remember to test the initial calling of pagination when there are <= paginationThresholdValue cells in the table (including zero). But if you think that just using the last cell is ok in most cases, I can merge this PR. Have a fantastic day! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Pagination offset (by which I mean the distance from the top message before we start loading more) used to be determined by the
offsetparameter passed toenableLoadMore, like so:This got changed in 568d00d. Now, we rely on
paginationTargetIndexPathinstead, which is hardcoded to be the last already loaded message:As a result,
pageSizeis no longer used. Remove it to make sure that consumers do not expect different behaviour when they modify the parameter.Comments
I was tempted to re-add that
offsetfunctionality, but this comment made me pause:I didn't quite understand it, is it saying that we don't want to call the pagination handler at all before we reach the last already loaded message?
Anyway, if you'd like me to look at adding that functionality back, I'd be more than happy to take a look, just didn't want to start making changes without understanding the history first :)