Fix scrollToObject: inconsistency bug when scrolling to bottom/right with content inset#1284
Closed
qhhonx wants to merge 4 commits intoInstagram:masterfrom
Closed
Fix scrollToObject: inconsistency bug when scrolling to bottom/right with content inset#1284qhhonx wants to merge 4 commits intoInstagram:masterfrom
qhhonx wants to merge 4 commits intoInstagram:masterfrom
Conversation
…lDirection:scrollPosition:animated:]` where the content inset(bottom/right) of the collection view was incorrectly being applied to the final offset and was inconsistent with the content inset(top/left) of the collection view being applied. [Qinghua Hong](https://github.com/xohozu)
Generated by 🚫 Danger |
rnystrom
suggested changes
Nov 28, 2018
Contributor
Author
|
Hi, @rnystrom , I can not figure out why the travis-ci task failed while my local building and testing are passed. Could you please check whether some common mistakes occurred? |
rnystrom
approved these changes
Jan 7, 2019
Contributor
rnystrom
left a comment
There was a problem hiding this comment.
@xohozu I think that's from stale Travis config, I'll take care of that. I think this is ready to import!
Contributor
facebook-github-bot
left a comment
There was a problem hiding this comment.
@rnystrom has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Contributor
|
@qhhonx has updated the pull request. Re-import the pull request |
Contributor
Author
@rnystrom Can you reimport this PR again? It just fixes testcase inconsistency without extra logic code. |
Contributor
Author
|
@lorixx It seems that you are active maintainer now. Can you review this PR and import it again? |
Contributor
facebook-github-bot
left a comment
There was a problem hiding this comment.
@bdotdub has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Contributor
bdotdub
added a commit
that referenced
this pull request
May 26, 2019
Just realized #1284 had an extraneous `else` block. This just cleans that up
4 tasks
facebook-github-bot
pushed a commit
that referenced
this pull request
May 28, 2019
Summary: Just realized #1284 had an extraneous `else` block. This just cleans that up ## Changes in this pull request See above ### Checklist - [X] All tests pass. Demo project builds and runs. - [X] I added tests, an experiment, or detailed why my change isn't tested. - [ ] I added an entry to the `CHANGELOG.md` for any breaking changes, enhancements, or bug fixes. - [X] I have reviewed the [contributing guide](https://github.com/Instagram/IGListKit/blob/master/.github/CONTRIBUTING.md) Pull Request resolved: #1325 Reviewed By: timonus Differential Revision: D15511564 Pulled By: timonus fbshipit-source-id: c21213ee6ccc06ffb0b646381cba3faff0347144
TimOliver
pushed a commit
that referenced
this pull request
Dec 10, 2025
…with content inset (#1284) Summary: The implementation of ```Objective-C -[IGListAdapter scrollToObject:supplementaryKinds:scrollDirection:scrollPosition:animated:] ``` has little inconsistencies where the `UICollectionViewScrollPositionLeft` and `UICollectionViewScrollPositionTop` considering the content inset left/top of the collection view was being applied to the final offset. However, the `UICollectionViewScrollPositionRight` and `UICollectionViewScrollPositionBottom` ignoring the content inset right/bottom of the collection view was being applied to the final offset. The different result is that scrolling to the most `Left/Top`, the first section content always be visible by considering its content inset while scrolling to the most `Right/Bottom`, the last section content always be fully displayed but not be fully visible by considering its content inset. ## Changes in this pull request Issue fixed: # ### Checklist - [x] All tests pass. Demo project builds and runs. - [x] I added tests, an experiment, or detailed why my change isn't tested. - [x] I added an entry to the `CHANGELOG.md` for any breaking changes, enhancements, or bug fixes. - [x] I have reviewed the [contributing guide](https://github.com/Instagram/IGListKit/blob/master/.github/CONTRIBUTING.md) Pull Request resolved: #1284 Reviewed By: lorixx Differential Revision: D13590416 Pulled By: lorixx fbshipit-source-id: ae52be9e5ba25b50c7a0ad768a4af728347523e2
TimOliver
pushed a commit
that referenced
this pull request
Dec 10, 2025
Summary: Just realized #1284 had an extraneous `else` block. This just cleans that up ## Changes in this pull request See above ### Checklist - [X] All tests pass. Demo project builds and runs. - [X] I added tests, an experiment, or detailed why my change isn't tested. - [ ] I added an entry to the `CHANGELOG.md` for any breaking changes, enhancements, or bug fixes. - [X] I have reviewed the [contributing guide](https://github.com/Instagram/IGListKit/blob/master/.github/CONTRIBUTING.md) Pull Request resolved: #1325 Reviewed By: timonus Differential Revision: D15511564 Pulled By: timonus fbshipit-source-id: c21213ee6ccc06ffb0b646381cba3faff0347144
TimOliver
pushed a commit
that referenced
this pull request
Dec 10, 2025
…with content inset (#1284) Summary: The implementation of ```Objective-C -[IGListAdapter scrollToObject:supplementaryKinds:scrollDirection:scrollPosition:animated:] ``` has little inconsistencies where the `UICollectionViewScrollPositionLeft` and `UICollectionViewScrollPositionTop` considering the content inset left/top of the collection view was being applied to the final offset. However, the `UICollectionViewScrollPositionRight` and `UICollectionViewScrollPositionBottom` ignoring the content inset right/bottom of the collection view was being applied to the final offset. The different result is that scrolling to the most `Left/Top`, the first section content always be visible by considering its content inset while scrolling to the most `Right/Bottom`, the last section content always be fully displayed but not be fully visible by considering its content inset. ## Changes in this pull request Issue fixed: # ### Checklist - [x] All tests pass. Demo project builds and runs. - [x] I added tests, an experiment, or detailed why my change isn't tested. - [x] I added an entry to the `CHANGELOG.md` for any breaking changes, enhancements, or bug fixes. - [x] I have reviewed the [contributing guide](https://github.com/Instagram/IGListKit/blob/master/.github/CONTRIBUTING.md) Pull Request resolved: #1284 Reviewed By: lorixx Differential Revision: D13590416 Pulled By: lorixx fbshipit-source-id: ae52be9e5ba25b50c7a0ad768a4af728347523e2
TimOliver
pushed a commit
that referenced
this pull request
Dec 10, 2025
Summary: Just realized #1284 had an extraneous `else` block. This just cleans that up ## Changes in this pull request See above ### Checklist - [X] All tests pass. Demo project builds and runs. - [X] I added tests, an experiment, or detailed why my change isn't tested. - [ ] I added an entry to the `CHANGELOG.md` for any breaking changes, enhancements, or bug fixes. - [X] I have reviewed the [contributing guide](https://github.com/Instagram/IGListKit/blob/master/.github/CONTRIBUTING.md) Pull Request resolved: #1325 Reviewed By: timonus Differential Revision: D15511564 Pulled By: timonus fbshipit-source-id: c21213ee6ccc06ffb0b646381cba3faff0347144
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
4 participants
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.
The implementation of
has little inconsistencies where the
UICollectionViewScrollPositionLeftandUICollectionViewScrollPositionTopconsidering the content inset left/top of the collection view was being applied to the final offset. However, theUICollectionViewScrollPositionRightandUICollectionViewScrollPositionBottomignoring the content inset right/bottom of the collection view was being applied to the final offset. The different result is that scrolling to the mostLeft/Top, the first section content always be visible by considering its content inset while scrolling to the mostRight/Bottom, the last section content always be fully displayed but not be fully visible by considering its content inset.Changes in this pull request
Issue fixed: #
Checklist
CHANGELOG.mdfor any breaking changes, enhancements, or bug fixes.