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

Added object to single section selection delegate callback #397

Closed
wants to merge 7 commits into from
Closed

Added object to single section selection delegate callback #397

wants to merge 7 commits into from

Conversation

Sherlouk
Copy link
Contributor

@Sherlouk Sherlouk commented Jan 9, 2017 •
edited
Loading

Changes in this pull request

  • Updated selectionDelegate to pass through the model object
  • Changelog entry under 3.0.0 as is breaking change

Try saying single section selection 10 times quick
Dabbing my left toe into some ObjC, so apologies if I violate years of rules

  • Update documentation internally

Closes #396

Pull request 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

@coveralls
Copy link

coveralls commented Jan 9, 2017 •
edited
Loading

Coverage Status

Coverage remained the same at 97.979% when pulling 277db9c on Sherlouk:selectionDelegate into c19379e on Instagram:master.

@facebook-github-bot
Copy link
Contributor

@Sherlouk updated the pull request - view changes

@jessesquires jessesquires mentioned this pull request Jan 9, 2017
Closed
@jessesquires
Copy link
Contributor

Thanks @Sherlouk ! 💯 Looks good to me.

This is a 3.0 breaking change though, I opened #398 to discuss.

cc @rnystrom

jessesquires
jessesquires reviewed Jan 9, 2017
View reviewed changes
Source/IGListSingleSectionController.h
@@ -45,8 +45,10 @@ typedef CGSize (^IGListSingleSectionCellSizeBlock)(id item, id<IGListCollectionC
Tells the delegate that the section controller was selected.

@param sectionController The section controller that was selected.
@param object The model for the given section
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: need a period . 😄

@facebook-github-bot
Copy link
Contributor

@Sherlouk updated the pull request - view changes

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.979% when pulling 866bee8 on Sherlouk:selectionDelegate into c19379e on Instagram:master.

@jessesquires jessesquires added this to the 3.0.0 milestone Jan 9, 2017
@Sherlouk
Copy link
Contributor Author

Travis/Tests are being a tad flakey, builds fine

As per discussion should be fine to merge into master

@facebook-github-bot
Copy link
Contributor

@Sherlouk updated the pull request - view changes

@facebook-github-bot
Copy link
Contributor

@Sherlouk updated the pull request - view changes

@jessesquires
Copy link
Contributor

Pushed a changelog update here, per #398

@jessesquires
Copy link
Contributor

@Sherlouk - question -- can you push your own branches to Insta/IGListKit instead using your fork?

That should be possible now. If so, let's do that. That way, we can all checkout any branch, push updates/fixes, etc.

@Sherlouk
Copy link
Contributor Author

@jesse_squires as per discussion in #399 I don't have write access yet but it's being worked on

Cc @rnystrom

@coveralls
Copy link

coveralls commented Jan 10, 2017 •
edited
Loading

Coverage Status

Coverage remained the same at 97.979% when pulling bd9f56a on Sherlouk:selectionDelegate into fb4e823 on Instagram:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.979% when pulling bd9f56a on Sherlouk:selectionDelegate into fb4e823 on Instagram:master.

@facebook-github-bot
Copy link
Contributor

@Sherlouk updated the pull request - view changes

@Sherlouk
Copy link
Contributor Author

Conflict resolution feature on GitHub is pretty cool

@coveralls
Copy link

coveralls commented Jan 11, 2017 •
edited
Loading

Coverage Status

Coverage remained the same at 98.065% when pulling 086f902 on Sherlouk:selectionDelegate into dd3b9ed on Instagram:master.

@facebook-github-bot
Copy link
Contributor

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

@jessesquires
Copy link
Contributor

oof. wanted to wait on merging this.

this is the only 3.0 change so far right? i'll see if i can fix up the branches

@jessesquires
Copy link
Contributor

ok. got everything sync'd up correctly on stable locally. now having GH permissions issues. will push once that's resolved

@rnystrom
Copy link
Contributor

@jessesquires ah sorry my bad, didn't see this needed to wait. Will report back on protected branch.

@PhilCai1993 PhilCai1993 mentioned this pull request Apr 1, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jessesquires jessesquires jessesquires left review comments

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

Successfully merging this pull request may close these issues.

IGListSingleSectionControllerDelegate selection function should include object
5 participants
@Sherlouk @coveralls @facebook-github-bot @jessesquires @rnystrom