Proper support vertical and bottom positions in IGListAdapter scrollToObject: API

[gmoledina]

Changes in this pull request

I simply wrote failing tests for scrollToObject API for the ScrollPosition parameter and then fixed the code.

If we agree that these tests are correct, I don't mind suggesting a fix for it. I think it's the indexPath's row that is hardcoded to 0 that needs to be changed depending on the scrollPosition desired. (Done!)

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

[rnystrom]

Dang, that Travis test output is garbage

  test_whenScrollVerticallyToItemWithPositionning, ((CGPointEqualToPoint([self.collectionView contentOffset], p)) equal to (YES)) failed: ("<00>") is not equal to ("<01>")

The expectation values are a little off, right? If section 2 has @100 then its got 100 100x10 items, so its 1,000 pts tall. So vertical should expect 455

// section 2 height
10 * 100 = 1,000

// center of section 2
1,010 / 2 = 505

// center of section 2 + section 1 offset
505 + 10 * 1 = 515

// subtract top half of viewport
515 - 100 / 2 = 465

And bottom should be:

// max Y of section 2 minus viewport
1,010 - 100 = 910

Right?

We found and fixed an issue when the content height doesn't fill the bounds: 1d926d9.

[gmoledina]

@rnystrom oh right !
For the last one, I agree it's 910. But for the one before, I think it should be 460 :

// section 2 height
10 * 100 = 1,000

// center of section 2
1,000 / 2 = 500  // minY = 0, maxY = 1000  (without section 1 offset)

// center of section 2 + section 1 offset
500 + 10 * 1 = 510

// subtract top half of viewport
510 - 100 / 2 = 460

Makes sense ? I updated the PR for now and will correct if I'm wrong.

[rnystrom]

@gmoledina duh, good call. You're right it should be 460. Sorry it took so long for me to get back! If you're still interested in doing this, want to take it on? That'd be awesome!

[facebook-github-bot]

@gmoledina updated the pull request - view changes

[facebook-github-bot]

@gmoledina updated the pull request - view changes

[gmoledina]

@rnystrom No Worries! Can't believe how easy the fix for this was ! My branch with the fix (+ extra tests) is ready (this same PR) but if you're reverting PR#808, then we'll have to wait as the fix also depends on the previous changes on the scrollToObject API.

[facebook-github-bot]

@gmoledina updated the pull request - view changes

[rnystrom]

Sounds great, will sort this out this week!

[rnystrom]

Restoring the base PR today! Will land soon, then we can do this. 💪

[facebook-github-bot]

@gmoledina updated the pull request - view changes

[rnystrom]

Waiting on CI, but including this in the 3.1 release

[rnystrom]

Need to run through some internal testing to make sure we don't break anything else. Importing, once everything is clear will land!

[facebook-github-bot]

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