Add storyboard support #39 #92

zhubofei · 2016-10-20T06:53:07Z

Changes in this pull request

This PR is for #39

Pull request checklist

I have reviewed the contributing guide

rnystrom · 2016-10-20T07:15:41Z

Wo! This and #90 arriving at the same time are so awesome. Thanks for taking this on! 💪

This solution is spot on, but we're going to need to add some tests. An example in the examples app would be awesome, but we could add that later. Take a look at #56 for an idea of what all we need:

Unit tests creating section controllers from a storyboard
Optional example
Add to CHANGELOG.md

A new initializer for IGListSingleSectionController

See

IGListKit/Source/IGListSingleSectionController.h

Lines 74 to 77 in ce4b851

    
           - (instancetype)initWithNibName:(NSString *)nibName 
        
                                    bundle:(nullable NSBundle *)bundle 
        
                            configureBlock:(IGListSingleSectionCellConfigureBlock)configureBlock 
        
                                 sizeBlock:(IGListSingleSectionCellSizeBlock)sizeBlock;

Note that we'll need unit tests for this too!

facebook-github-bot · 2016-10-20T08:34:03Z

@zhubofei updated the pull request - view changes

zhubofei · 2016-10-20T08:45:45Z

@rnystrom Is a new initializer for IGListSingleSectionController necessary?
Do you mean something like

- (instancetype)initWithStoryboardCollectionView:(UICollectionView *)collectionView
                         identifier:(NSString *)identifier
                 configureBlock:(IGListSingleSectionCellConfigureBlock)configureBlock
                      sizeBlock:(IGListSingleSectionCellSizeBlock)sizeBlock;

?
I'm not sure if sending in a weak ref of collectionView is a good idea. 🤔

rnystrom · 2016-10-20T08:49:13Z

@zhubofei if we don't add a new init, how could we use IGListSingleSectionController with storyboards?

We shouldn't need to pass the collection view to the section controller, right? Something like

- (instancetype)initWithStoryboardIdentifier:(NSString *)identifier 
  configureBlock:(IGListSingleSectionCellConfigureBlock)configureBlock
  sizeBlock:(IGListSingleSectionCellSizeBlock)sizeBlock;

should be fine I think.

Also looks like there are some merge conflicts w/ the example project. Probably want to rebase on master.

zhubofei · 2016-10-20T08:59:38Z

@rnystrom I think the collectionView could be prototyped in the storyboard as well? If we init the collectionView again in the SingleSectionController, we will lose the constraints attached to the original one, right?

# Conflicts: # Example/IGListKitExamples.xcodeproj/project.pbxproj # Example/IGListKitExamples/ViewControllers/DemosViewController.swift

facebook-github-bot · 2016-10-20T09:10:07Z

@zhubofei updated the pull request - view changes

rnystrom · 2016-10-20T09:13:24Z

@zhubofei oh the collection view should definitely be created from the storyboard too. Though you would just make the collection view subclass IGListCollectionView in the storyboard. Then just

@property @IBOutlet IGListCollectionView *collectionView;

// viewDidLoad
self.adapter.collectionView = self.collectionView;

That'll use the storyboard-generated collection view for everything then.

Make sense?

zhubofei · 2016-10-20T09:17:44Z

Right! Sorry, I was just being silly 😂.

facebook-github-bot · 2016-10-20T12:30:59Z

@zhubofei updated the pull request - view changes

facebook-github-bot · 2016-10-20T12:48:13Z

@zhubofei updated the pull request - view changes

rnystrom · 2016-10-20T13:20:44Z

Source/IGListSingleSectionController.h

+ */
+- (instancetype)initWithStoryboardCellIdentifier:(NSString *)identifier
+                              configureBlock:(IGListSingleSectionCellConfigureBlock)configureBlock
+                                   sizeBlock:(IGListSingleSectionCellSizeBlock)sizeBlock;


nit: mind aligning the colons?

rnystrom · 2016-10-20T13:21:13Z

Source/IGListSingleSectionController.m

+- (instancetype)initWithStoryboardCellIdentifier:(NSString *)identifier
+                              configureBlock:(IGListSingleSectionCellConfigureBlock)configureBlock
+                                   sizeBlock:(IGListSingleSectionCellSizeBlock)sizeBlock {
+    IGParameterAssert(identifier != nil);


Let's assert identifier.length > 0

rnystrom · 2016-10-20T13:21:40Z

Source/IGListSingleSectionController.m

+    IGParameterAssert(identifier != nil);
+    IGParameterAssert(configureBlock != nil);
+    if (self = [super init]) {
+        _identifier = identifier;


_identifier = [identifier copy];

rnystrom · 2016-10-20T13:22:18Z

Source/IGListSingleSectionController.m

+                              configureBlock:(IGListSingleSectionCellConfigureBlock)configureBlock
+                                   sizeBlock:(IGListSingleSectionCellSizeBlock)sizeBlock {
+    IGParameterAssert(identifier != nil);
+    IGParameterAssert(configureBlock != nil);


Need an assert for sizeBlock too

@rnystrom What about the other two init funcions initWithCellClass: & initWithNibName:? Do you want me to add assertions for sizeBlock to them as well?

rnystrom · 2016-10-20T13:26:46Z

Source/IGListSingleSectionController.m

+    } else if ([self.identifier length] > 0) {
+        cell = [collectionContext dequeueReusableCellFromStoryboardWithIdentifier:self.identifier
+                                            forSectionController:self
+                                                         atIndex:index];


nit: colon alignment

rnystrom · 2016-10-20T13:30:55Z

Tests/Objects/IGTestSingleStoryboardViewController.m

+//
+//  Created by Bofei Zhu on 10/20/16.
+//  Copyright © 2016 Instagram. All rights reserved.
+//


Needs our copyright comments

rnystrom · 2016-10-20T13:31:04Z

Tests/Objects/IGTestStoryboardCell.h

+//
+//  Created by Bofei Zhu on 10/20/16.
+//  Copyright © 2016 Instagram. All rights reserved.
+//


++copyright comments

rnystrom · 2016-10-20T13:31:08Z

Tests/Objects/IGTestStoryboardCell.m

+//
+//  Created by Bofei Zhu on 10/20/16.
+//  Copyright © 2016 Instagram. All rights reserved.
+//


rnystrom · 2016-10-20T13:32:00Z

Tests/Objects/IGTestStoryboardCell.m

+#import "IGTestStoryboardCell.h"
+
+@implementation IGTestStoryboardCell
+


Probably need to init the label at least? 🤔

Since it is done in Storyboard, I think we don't need to call something like [[UILabel alloc] initWithFrame:CGRectMake(0.0, 0.0, width, height)] to init here.

jessesquires · 2016-10-20T15:35:57Z

Source/IGListCollectionContext.h

+ */
+- (__kindof UICollectionViewCell *)dequeueReusableCellFromStoryboardWithIdentifier:(NSString *)identifier
+                                                              forSectionController:(IGListSectionController <IGListSectionType> *)sectionController
+                                                                           atIndex:(NSInteger)index;


I see this added to the .h file, but not IGListCollectionContext.m

... am I missing something?? 🤔

@jessesquires There seems to be no implementation forIGListCollectionContext. 😶

Yup just a protocol

lol. derp.

implemented in IGListAdapter 🙃

jessesquires · 2016-10-20T15:40:11Z

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

facebook-github-bot · 2016-10-20T23:18:41Z

@zhubofei updated the pull request - view changes

facebook-github-bot · 2016-10-20T23:19:05Z

@zhubofei updated the pull request - view changes

facebook-github-bot · 2016-10-20T23:24:35Z

@zhubofei updated the pull request - view changes

facebook-github-bot · 2016-10-20T23:26:57Z

@zhubofei updated the pull request - view changes

facebook-github-bot · 2016-10-20T23:35:56Z

@zhubofei updated the pull request - view changes

facebook-github-bot · 2016-10-21T14:36:06Z

@zhubofei updated the pull request - view changes

JakeLin

Great PR. 👍

JakeLin · 2016-10-23T03:28:39Z

Example/IGListKitExamples/SectionControllers/DemoSectionController.swift

+            let storyboard = UIStoryboard(name: "Demo", bundle: nil)
+            let controller = storyboard.instantiateViewController(withIdentifier: "singleSectionDemo") as! SingleSectionStoryboardViewController
+            controller.title = object?.name
+            viewController?.navigationController?.pushViewController(controller, animated: true)


Is it possible we can encapsulate those two branches in a single method, e.g. controllerFromStorybard(withIdentifier identifier: String) or pushToViewControllerFromStoryboard(withIdentifier identifier: String)

@JakeLin I thought about this for some time before putting it this way. Because it really makes no sense to give the VC identifier the same string as VC title, or does it? So if we combine this two together, I think we need a dictionary for title->identifier, right?

Or shall we add a VC identifier property to the DemoItem class?

@JakeLin I added a nullable controllerIdentifier property to DemoItem class. I think it looks much neater now.

++ great ideas here

JakeLin · 2016-10-23T03:30:44Z

Example/IGListKitExamples/ViewControllers/SingleSectionStoryboardViewController.swift

+    override func viewDidLoad() {
+        super.viewDidLoad()
+
+        view.addSubview(collectionView)


Since we have storyboard, do we need to addSubview here?

JakeLin · 2016-10-23T07:19:52Z

Example/IGListKitExamples/ViewControllers/SingleSectionStoryboardViewController.swift

+
+    override func viewDidLayoutSubviews() {
+        super.viewDidLayoutSubviews()
+        collectionView.frame = view.bounds


If we use Storyboard, can we use Auto Layout or autoresizing? Then we don't need this line.

@JakeLin Auto Layout will give collectionView a weird top margin to the navbar. I can try to fix this, but I don't think I can do it without adding more code.

@zhubofei Do you mean the topLayoutGuide? I am not sure do we always want the collection view stretch to the parent view? In the current implementation, the Auto Layout constraints will not work anymore since we always reset the frame of the collection view.

@JakeLin Nevermind. I find out I set a wrong constraint😂. Will definitely remove this line.

JakeLin · 2016-10-23T07:20:09Z

Example/IGListKitExamples/ViewControllers/SingleSectionStoryboardViewController.swift

+        }
+
+        let sizeBlock = { (context: IGListCollectionContext?) -> CGSize in
+            guard let context = context else { return CGSize() }


To make it more specific, I think return .zero is better.

JakeLin · 2016-10-23T07:21:13Z

Tests/Objects/IGTestStoryboardCell.m

+#import "IGTestStoryboardCell.h"
+
+@implementation IGTestStoryboardCell
+


Since it is done in Storyboard, I think we don't need to call something like [[UILabel alloc] initWithFrame:CGRectMake(0.0, 0.0, width, height)] to init here.

facebook-github-bot · 2016-10-23T08:23:59Z

@zhubofei updated the pull request - view changes

facebook-github-bot · 2016-10-23T08:27:59Z

@zhubofei updated the pull request - view changes

facebook-github-bot · 2016-10-23T08:34:22Z

@zhubofei updated the pull request - view changes

JakeLin · 2016-10-23T10:38:02Z

@zhubofei I like the idea to have controllerIdentifier in DemoSectionController 👍

rnystrom

Looks good! I have a couple nits but they aren't blocking. I'm going to start the import process! Congrats on this awesome feature!

rnystrom · 2016-10-24T00:15:47Z

Example/IGListKitExamples/SectionControllers/DemoSectionController.swift

@@ -53,7 +56,12 @@ class DemoSectionController: IGListSectionController, IGListSectionType {
    }

    func didSelectItem(at index: Int) {
-        if let controller = object?.controllerClass.init() {
+        if let identifier = object?.controllerIdentifier {
+            let storyboard = UIStoryboard(name: "Demo", bundle: nil)


Noting to myself: this means that any demo using a storyboard will have to put their VCs in the Demo.storyboard file. I think that's fine for now.

rnystrom · 2016-10-24T00:16:07Z

Example/IGListKitExamples/SectionControllers/DemoSectionController.swift


    init(
        name: String,
-        controllerClass: UIViewController.Type
+        controllerClass: UIViewController.Type,
+        controllerIdentifier: String?=nil


ultra nit: spaces String? = nil

rnystrom · 2016-10-24T00:17:43Z

Example/IGListKitExamples/ViewControllers/SingleSectionStoryboardViewController.swift

+        let alert = UIAlertController(title: "Section \(section) was selected 🎉", message: nil, preferredStyle: .alert)
+        alert.addAction(UIAlertAction(title: "Dismiss", style: .default, handler: nil))
+        present(alert, animated: true, completion: nil)
+    }


This is awesome

rnystrom · 2016-10-24T00:18:13Z

Example/IGListKitExamples/ViewControllers/SingleSectionStoryboardViewController.swift

+
+    func emptyView(for listAdapter: IGListAdapter) -> UIView? {
+        return nil
+    }


style nit: other VC uses { return nil }

rnystrom · 2016-10-24T00:18:56Z

Source/IGListAdapter.m

+                                                                           atIndex:(NSInteger)index {
+    IGAssertMainThread();
+    IGParameterAssert(sectionController != nil);
+    IGParameterAssert(identifier != nil);


This should be IGParameterAssert(identifier.length > 0);

facebook-github-bot · 2016-10-24T00:24:29Z

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

facebook-github-bot · 2016-10-24T03:45:54Z

@zhubofei updated the pull request - view changes - changes since last import

coveralls · 2016-10-24T03:51:13Z

Changes Unknown when pulling df586a1 on zhubofei:storyboard into * on Instagram:master*.

zhubofei · 2016-10-26T00:12:38Z

AWESOME!

jessesquires · 2016-10-26T01:13:00Z

🎉

JakeLin · 2016-10-26T02:11:12Z

Great PR to unlock Storyboard

Summary: Miss this in the previous PR #92. Will add unit test soon. Closes #123 Differential Revision: D4101594 Pulled By: rnystrom fbshipit-source-id: 820030358532b0878f6d9e9092834266c9260a38

Add storyboard support

fb4dae1

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

rnystrom assigned zhubofei Oct 20, 2016

Add storyboard example

38721a6

Merge remote-tracking branch 'Instagram/master' into storyboard

e9c4f6d

# Conflicts: # Example/IGListKitExamples.xcodeproj/project.pbxproj # Example/IGListKitExamples/ViewControllers/DemosViewController.swift

Support Single Section Controller & Tests

098a7db

Don't need to reset collectionView's bound

a2cde80

rnystrom suggested changes Oct 20, 2016

View reviewed changes

jessesquires reviewed Oct 20, 2016

View reviewed changes

jessesquires added this to the 2.0.0 milestone Oct 20, 2016

Style Fix

9b680db

Merge remote-tracking branch 'Instagram/master' into storyboard

720a022

Update CHANGLOG

1fd2f38

zhubofei force-pushed the storyboard branch from 49c98da to 1fd2f38 Compare October 20, 2016 23:35

Update documentation

6bdd64c

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

zhubofei closed this Oct 22, 2016

zhubofei reopened this Oct 22, 2016

JakeLin approved these changes Oct 23, 2016

View reviewed changes

Merge remote-tracking branch 'Instagram/master' into storyboard

9d31a1a

zhubofei force-pushed the storyboard branch from eea57dd to 359af50 Compare October 23, 2016 08:27

Tweak storyboard examples

55bc6ba

zhubofei force-pushed the storyboard branch from 359af50 to 55bc6ba Compare October 23, 2016 08:31

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

rnystrom approved these changes Oct 24, 2016

View reviewed changes

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

nit

df586a1

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

facebook-github-bot closed this in 8855b51 Oct 25, 2016

zhubofei mentioned this pull request Oct 26, 2016

Support supplementaryViews created from storyboard #123

Closed

zhubofei deleted the storyboard branch November 1, 2016 18:50

dcaunt mentioned this pull request Dec 2, 2016

Init overrides storyboard settings in IGListCollectionView. Should -[IGListCollectionView commonInit] be removed? #277

Closed

2 tasks

		#import "IGTestStoryboardCell.h"

		@implementation IGTestStoryboardCell

Add storyboard support #39 #92

Add storyboard support #39 #92

Conversation

zhubofei commented Oct 20, 2016

Changes in this pull request

Pull request checklist

rnystrom commented Oct 20, 2016

facebook-github-bot commented Oct 20, 2016

zhubofei commented Oct 20, 2016 • edited Loading

rnystrom commented Oct 20, 2016

zhubofei commented Oct 20, 2016

facebook-github-bot commented Oct 20, 2016

rnystrom commented Oct 20, 2016

zhubofei commented Oct 20, 2016

facebook-github-bot 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

zhubofei Oct 20, 2016 • edited Loading

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

Choose a reason for hiding this comment

zhubofei Oct 20, 2016 • edited Loading

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

facebook-github-bot commented Oct 20, 2016

facebook-github-bot commented Oct 20, 2016

facebook-github-bot commented Oct 20, 2016

facebook-github-bot commented Oct 20, 2016

facebook-github-bot commented Oct 21, 2016

JakeLin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhubofei Oct 23, 2016 • edited Loading

Choose a reason for hiding this comment

zhubofei Oct 23, 2016 • edited Loading

Choose a reason for hiding this comment

zhubofei Oct 23, 2016 • edited Loading

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

Choose a reason for hiding this comment

facebook-github-bot commented Oct 23, 2016

facebook-github-bot commented Oct 23, 2016

facebook-github-bot commented Oct 23, 2016

JakeLin commented Oct 23, 2016

rnystrom left a 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 24, 2016

facebook-github-bot commented Oct 24, 2016

coveralls commented Oct 24, 2016 • edited Loading

zhubofei commented Oct 26, 2016

jessesquires commented Oct 26, 2016

JakeLin commented Oct 26, 2016

zhubofei commented Oct 20, 2016 •

edited

Loading

zhubofei Oct 20, 2016 •

edited

Loading

zhubofei Oct 20, 2016 •

edited

Loading

zhubofei Oct 23, 2016 •

edited

Loading

zhubofei Oct 23, 2016 •

edited

Loading

zhubofei Oct 23, 2016 •

edited

Loading

coveralls commented Oct 24, 2016 •

edited

Loading