Support supplementaryViews created from nibs #90

rawlinxx · 2016-10-20T05:50:31Z

Changes in this pull request

Summary: add dequeueReusableSupplementaryView with nib

facebook-github-bot · 2016-10-20T05:51:03Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions.

rnystrom · 2016-10-20T06:11:07Z

@rawlinxx this is awesome! Thanks for taking this on.

In order to merge this, we're going to have to add some unit tests. Take a look at #56 for an example of requirements for this.

facebook-github-bot · 2016-10-20T06:14:03Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

rnystrom · 2016-10-20T06:16:17Z

Source/IGListCollectionContext.h

+- (__kindof UICollectionReusableView *)dequeueReusableSupplementaryViewOfKind:(NSString *)elementKind
+                                                         forSectionController:(IGListSectionController <IGListSectionType> *)sectionController
+                                                                      nibName:(NSString *)nibName
+                                                                       bundle:(NSBundle *)bundle


We'll need this to be nullable: bundle:(nullable NSBundle *)bundle

right! just like the documentation i copied said :)

rnystrom · 2016-10-20T06:18:56Z

Source/IGListAdapter.m

+                                                                      nibName:(NSString *)nibName
+                                                                       bundle:(NSBundle *)bundle
+                                                                      atIndex:(NSInteger)index {
+    IGAssertMainThread();


Let's also add:

IGParameterAssert([nibName length] > 0); IGParameterAssert([elementKind length] > 0);

I realize those are also missing from other methods. Adding some starter tasks for that.

bundle should be nullable here, right?

Sorry for that, I'm trying this framework, hope to have this feature ASAP, without attention to details

@rawlinxx no worries! PRs are an iterative process, its good to get something up and we can all work on it together 👨‍👩‍👧‍👦

rnystrom · 2016-10-20T06:19:54Z

Source/IGListStackedSectionController.m

+                                                                      atIndex:(NSInteger)index
+{
+    const NSUInteger offset = [self offsetForSectionController:sectionController];
+    return (UICollectionViewCell *_Nonnull)[self.collectionContext dequeueReusableSupplementaryViewOfKind:elementKind


Do we need this cast?

I do not quite understand why (UICollectionViewCell *_Nonnull) is needed here, just following the previous method implementation since I have not read all the source code yet, i think UICollectionReusableView is more suitable here, am i wrong?

jessesquires · 2016-10-20T15:39:42Z

Also need to update the CHANGELOG.md -- add an entry under the 2.0.0 release 😄

facebook-github-bot · 2016-10-20T16:37:48Z

@rawlinxx updated the pull request - view changes

rnystrom · 2016-10-21T07:09:34Z

Source/IGListAdapter.m

@@ -759,6 +761,25 @@ - (__kindof UICollectionReusableView *)dequeueReusableSupplementaryViewOfKind:(N
    return [collectionView dequeueReusableSupplementaryViewOfKind:elementKind withReuseIdentifier:identifier forIndexPath:indexPath];
 }

+- (__kindof UICollectionReusableView *)dequeueReusableSupplementaryViewOfKind:(NSString *)elementKind
+                                                         forSectionController:(IGListSectionController <IGListSectionType> *)sectionController


nit: let's remove the space between IGListSectionController<IGListSectionType>

rnystrom · 2016-10-21T07:10:01Z

Source/IGListCollectionContext.h

+ @note This method uses a string representation of the view class as the identifier.
+ */
+- (__kindof UICollectionReusableView *)dequeueReusableSupplementaryViewOfKind:(NSString *)elementKind
+                                                         forSectionController:(IGListSectionController <IGListSectionType> *)sectionController


nit: spacing between class and protocol

rnystrom · 2016-10-21T07:11:49Z

Source/IGListStackedSectionController.m

                                                                                     forSectionController:self
                                                                                                    class:viewClass
                                                                                                  atIndex:(index + offset)];
 }

+- (UICollectionReusableView *)dequeueReusableSupplementaryViewOfKind:(NSString *)elementKind
+                                                forSectionController:(IGListSectionController <IGListSectionType> *)sectionController


spacing between class/protocol

rnystrom · 2016-10-21T07:12:30Z

Source/IGListStackedSectionController.m

+                                                                                     forSectionController:self
+                                                                                                  nibName:nibName
+                                                                                                   bundle:bundle
+                                                                                                  atIndex:(index + offset)];


can we realign the colons?

rnystrom · 2016-10-21T07:13:01Z

Source/IGListStackedSectionController.m

+                                                             nibName:(NSString *)nibName
+                                                              bundle:(NSBundle *)bundle
+                                                             atIndex:(NSInteger)index
+{


curly needs to be on the same line as the last param

rnystrom · 2016-10-21T07:15:46Z

Source/IGListStackedSectionController.m

+                                                             atIndex:(NSInteger)index
+{
+    const NSUInteger offset = [self offsetForSectionController:sectionController];
+    return (UICollectionReusableView *_Nonnull)[self.collectionContext dequeueReusableSupplementaryViewOfKind:elementKind


i think UICollectionReusableView is more suitable here, am i wrong?

Hmm I'm actually not sure why we're casting it in the other methods. I just tried it locally and we can remove those. I wouldn't worry about it now. Can we revert the UICollectionReusableView changes? We can do cleanup in another PR.

facebook-github-bot · 2016-10-21T10:49:42Z

@rawlinxx updated the pull request - view changes

# Conflicts: # Source/IGListAdapter.m

facebook-github-bot · 2016-10-24T12:18:01Z

@rawlinxx updated the pull request - view changes

coveralls · 2016-10-24T12:25:00Z

Coverage decreased (-0.8%) to 85.123% when pulling 17bb25e on rawlinxx:master into 8ccdc83 on Instagram:master.

rnystrom · 2016-10-27T16:45:59Z

@rawlinxx Circling back to this, we wont be able to land until we add some unit tests. #56 added nib support for cells along w/ some unit tests. If we can basically copy exactly what we added there we'll be able to merge!

zhubofei · 2016-11-03T14:27:41Z

Any update on this PR 🤔?

jessesquires · 2016-11-03T14:48:45Z

Re-opening. @rawlinxx - was this closed by mistake?

jessesquires · 2016-11-03T14:49:37Z

@rnystrom - If needed, we can push commits directly to this PR for tests and carry it forward.

jessesquires · 2016-11-05T15:35:07Z

We're carrying these changes forward with some small modifications/additions in #162.

Closing in favor of #162.

Summary: Continuing the work on #90. I don't believe I can push directly to that PR since the origin is `master` of a repo I don't have access to. https://help.github.com/articles/checking-out-pull-requests-locally/ I went ahead and added another supplementary view test copying the old one we had. cc jessesquires in case there's something else I can do here. I believe this will still give rawlinxx credit? Closes #162 Differential Revision: D4137364 Pulled By: rnystrom fbshipit-source-id: d8418ac5728fd6d9570fa1d1568f4343f5c4112b

Support supplementaryViews created from nibs

2eaf0f1

facebook-github-bot added the CLA Signed label Oct 20, 2016

rnystrom suggested changes Oct 20, 2016

View reviewed changes

This was referenced Oct 20, 2016

Add more asserts for view dequeueing in IGListAdapter #91

Closed

Add storyboard support #39 #92

Closed

rnystrom assigned rawlinxx Oct 20, 2016

jessesquires added this to the 2.0.0 milestone Oct 20, 2016

complete

7111bb5

facebook-github-bot added the CLA Signed label Oct 21, 2016

rnystrom suggested changes Oct 21, 2016

View reviewed changes

complete

1dbabbb

facebook-github-bot added the CLA Signed label Oct 22, 2016

Merge branch 'master' of https://github.com/Instagram/IGListKit

17bb25e

# Conflicts: # Source/IGListAdapter.m

facebook-github-bot added the CLA Signed label Oct 24, 2016

rawlinxx closed this Nov 3, 2016

jessesquires reopened this Nov 3, 2016

rnystrom mentioned this pull request Nov 4, 2016

Support supplementaryViews created from nibs (continued) #162

Closed

jessesquires closed this Nov 5, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support supplementaryViews created from nibs #90

Support supplementaryViews created from nibs #90

rawlinxx commented Oct 20, 2016

facebook-github-bot commented Oct 20, 2016

rnystrom commented Oct 20, 2016

facebook-github-bot commented Oct 20, 2016

rnystrom Oct 20, 2016

rawlinxx Oct 20, 2016

rnystrom Oct 20, 2016

jessesquires Oct 20, 2016

rawlinxx Oct 20, 2016

rnystrom Oct 21, 2016

rnystrom Oct 20, 2016

rawlinxx Oct 20, 2016

jessesquires commented Oct 20, 2016

facebook-github-bot commented Oct 20, 2016

rnystrom Oct 21, 2016

rnystrom Oct 21, 2016

rnystrom Oct 21, 2016

rnystrom Oct 21, 2016

rnystrom Oct 21, 2016

rnystrom Oct 21, 2016

rawlinxx Oct 21, 2016

facebook-github-bot commented Oct 21, 2016

facebook-github-bot commented Oct 24, 2016

coveralls commented Oct 24, 2016 •

edited

Loading

rnystrom commented Oct 27, 2016

zhubofei commented Nov 3, 2016

jessesquires commented Nov 3, 2016

jessesquires commented Nov 3, 2016

jessesquires commented Nov 5, 2016

Support supplementaryViews created from nibs #90

Support supplementaryViews created from nibs #90

Conversation

rawlinxx commented Oct 20, 2016

Changes in this pull request

facebook-github-bot commented Oct 20, 2016

rnystrom commented Oct 20, 2016

facebook-github-bot commented Oct 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessesquires commented Oct 20, 2016

facebook-github-bot commented Oct 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Oct 21, 2016

facebook-github-bot commented Oct 24, 2016

coveralls commented Oct 24, 2016 • edited Loading

rnystrom commented Oct 27, 2016

zhubofei commented Nov 3, 2016

jessesquires commented Nov 3, 2016

jessesquires commented Nov 3, 2016

jessesquires commented Nov 5, 2016

coveralls commented Oct 24, 2016 •

edited

Loading