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.

Sign up for GitHub

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

Jump to bottom

Specify end position for -scrollToObject #196

Closed
wants to merge 11 commits into from
Closed

Specify end position for -scrollToObject #196

wants to merge 11 commits into from

Conversation

zhubofei
Copy link

@zhubofei zhubofei commented Nov 13, 2016 •
edited
Loading

@coveralls
Copy link

coveralls commented Nov 13, 2016 •
edited
Loading

Coverage Status

Coverage decreased (-0.4%) to 91.069% when pulling 634d2ea on zhubofei:scroll-position into 8ea5e08 on Instagram:master.

@facebook-github-bot
Copy link
Contributor

@zhubofei updated the pull request - view changes

@coveralls
Copy link

coveralls commented Nov 13, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.2%) to 91.667% when pulling dfe8f02 on zhubofei:scroll-position into 8ea5e08 on Instagram:master.

@facebook-github-bot
Copy link
Contributor

@zhubofei updated the pull request - view changes

@coveralls
Copy link

coveralls commented Nov 14, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.2%) to 91.667% when pulling 6c9df5e on zhubofei:scroll-position into 8ea5e08 on Instagram:master.

@facebook-github-bot
Copy link
Contributor

@zhubofei updated the pull request - view changes

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 91.667% when pulling d2f4f5f on zhubofei:scroll-position into 8ea5e08 on Instagram:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 91.667% when pulling d2f4f5f on zhubofei:scroll-position into 8ea5e08 on Instagram:master.

@zhubofei zhubofei closed this Nov 14, 2016
@zhubofei zhubofei reopened this Nov 14, 2016
@coveralls
Copy link

coveralls commented Nov 14, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.2%) to 91.667% when pulling d2f4f5f on zhubofei:scroll-position into 8ea5e08 on Instagram:master.

@rnystrom
Copy link
Contributor

This is awesome. Since its a useful and breaking change, let's add this to 2.0. @zhubofei can you update CHANGELOG.md w/ breaking changes for this?

@rnystrom rnystrom added this to the 2.0.0 milestone Nov 16, 2016
Bofei Zhu added 3 commits November 16, 2016 16:58
Update CHANGELOG
4a7db32
Merge
73ab8d9 
# Conflicts:
#	CHANGELOG.md
grammar
d6e5846
rnystrom
rnystrom suggested changes Nov 16, 2016
View reviewed changes
CHANGELOG.md
@@ -11,6 +11,7 @@ This release closes the [2.0.0 milestone](https://github.com/Instagram/IGListKit

- Diff result method `-resultWithUpdatedMovesAsDeleteInserts` removed and replaced with `-resultForBatchUpdates` [(b5aa5e3)](https://github.com/Instagram/IGListKit/commit/b5aa5e39002854c947e777c11ae241f67f24d19c)
- `IGListDiffable` equality method changed from `isEqual:` to `isEqualToDiffableObject:`
- `ScrollToObject` method changed from `-scrollToObject:supplementaryKinds:scrollDirection:animated` to `-scrollToObject:supplementaryKinds:scrollDirection:atScrollPosition:animated`. Added support for specify end position. [Bofei Zhu](https://github.com/zhubofei) [(#196)](https://github.com/Instagram/IGListKit/pull/196)
Copy link
Contributor

Choose a reason for hiding this comment

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

super nit: Can we remove the ScrollToObject formatting and just start w/ "Scrolling method changed from"

@facebook-github-bot
Copy link
Contributor

@zhubofei updated the pull request - view changes

rnystrom
rnystrom reviewed Nov 16, 2016
View reviewed changes
Tests/IGListAdapterTests.m
IGAssertEqualPoint([self.collectionView contentOffset], 500, 0);
[self.adapter scrollToObject:@6 supplementaryKinds:nil scrollDirection:UICollectionViewScrollDirectionHorizontal atScrollPosition:UICollectionViewScrollPositionCenteredHorizontally animated:NO];
IGAssertEqualPoint([self.collectionView contentOffset], 500, 0);
[self.adapter scrollToObject:@6 supplementaryKinds:nil scrollDirection:UICollectionViewScrollDirectionHorizontal atScrollPosition:UICollectionViewScrollPositionRight animated:NO];
IGAssertEqualPoint([self.collectionView contentOffset], 500, 0);
self.layout.scrollDirection = UICollectionViewScrollDirectionVertical;
Copy link
Contributor

Choose a reason for hiding this comment

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

I just realized that these tests don't actually test the position since items are full width. We almost need a new test suite to actually cover these scenarios.

@jessesquires what do you think?

Copy link
Author

Choose a reason for hiding this comment

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

Yep 😂

Copy link
Contributor

Choose a reason for hiding this comment

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

@rnystrom lol. yeah, lets:

  1. go ahead and merge this
  2. open a new issue to track this and follow-up there (and eventually submit new PR)

Copy link
Author

Choose a reason for hiding this comment

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

@rnystrom We need new sectionController and new datasource. I think it would be much easier if we can fix #183 first.

Merge
08c08aa 
# Conflicts:
#	CHANGELOG.md
@jessesquires
Copy link
Contributor

@zhubofei -- Looks like we need to resolve conflicts in CHANGELOG.md, but it should be pretty easy. 😄

After that, I think we're good here 👍

@facebook-github-bot
Copy link
Contributor

@zhubofei updated the pull request - view changes

@zhubofei
Copy link
Author

@jessesquires g2g 💪💪

@zhubofei zhubofei closed this Nov 17, 2016
@zhubofei zhubofei reopened this Nov 17, 2016
@facebook-github-bot
Copy link
Contributor

@zhubofei updated the pull request - view changes

@facebook-github-bot
Copy link
Contributor

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

@rnystrom rnystrom mentioned this pull request Nov 18, 2016
Closed
@jessesquires
Copy link
Contributor

Thanks @zhubofei ! 💯

@zhubofei zhubofei deleted the scroll-position branch November 18, 2016 19:31
This was referenced Nov 21, 2016
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rnystrom rnystrom rnystrom approved these changes

@jessesquires jessesquires jessesquires approved these changes

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
2.0.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@zhubofei @coveralls @facebook-github-bot @rnystrom @jessesquires @bofeizhu