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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix scrollToObject: inconsistency bug when scrolling to bottom/right with content inset #1284

Closed
wants to merge 4 commits into from

Conversation

qhhonx
Copy link
Contributor

@qhhonx qhhonx commented Nov 28, 2018

The implementation of

-[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

  • All tests pass. Demo project builds and runs.
  • 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.
  • I have reviewed the contributing guide

…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)
@iglistkit-bot
Copy link

1 Warning
⚠️ All pull requests should have a milestone attached, unless marked #trivial.

Generated by 🚫 Danger

Tests/IGListAdapterTests.m Outdated Show resolved Hide resolved
Tests/IGListAdapterTests.m Outdated Show resolved Hide resolved
Source/IGListAdapter.m Show resolved Hide resolved
@qhhonx
Copy link
Contributor Author

qhhonx commented Dec 4, 2018

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?

Copy link
Contributor

@rnystrom rnystrom left a comment

Choose a reason for hiding this comment

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

@xohozu I think that's from stale Travis config, I'll take care of that. I think this is ready to import!

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@rnystrom has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@qhhonx has updated the pull request. Re-import the pull request

@qhhonx
Copy link
Contributor Author

qhhonx commented May 24, 2019

@qhhonx has updated the pull request. Re-import the pull request

@rnystrom Can you reimport this PR again? It just fixes testcase inconsistency without extra logic code.

@qhhonx
Copy link
Contributor Author

qhhonx commented May 25, 2019

@lorixx It seems that you are active maintainer now. Can you review this PR and import it again?

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@bdotdub has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@lorixx merged this pull request in 619b835.

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
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
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.

None yet

4 participants