-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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.
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
Add storyboard support #39 #92
Conversation
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:
|
@zhubofei updated the pull request - view changes |
@rnystrom Is a new initializer for - (instancetype)initWithStoryboardCollectionView:(UICollectionView *)collectionView
identifier:(NSString *)identifier
configureBlock:(IGListSingleSectionCellConfigureBlock)configureBlock
sizeBlock:(IGListSingleSectionCellSizeBlock)sizeBlock; ? |
@zhubofei if we don't add a new init, how could we use 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. |
@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
@zhubofei updated the pull request - view changes |
@zhubofei oh the collection view should definitely be created from the storyboard too. Though you would just make the collection view subclass
That'll use the storyboard-generated collection view for everything then. Make sense? |
Right! Sorry, I was just being silly 😂. |
@zhubofei updated the pull request - view changes |
@zhubofei updated the pull request - view changes |
*/ | ||
- (instancetype)initWithStoryboardCellIdentifier:(NSString *)identifier | ||
configureBlock:(IGListSingleSectionCellConfigureBlock)configureBlock | ||
sizeBlock:(IGListSingleSectionCellSizeBlock)sizeBlock; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: mind aligning the colons?
- (instancetype)initWithStoryboardCellIdentifier:(NSString *)identifier | ||
configureBlock:(IGListSingleSectionCellConfigureBlock)configureBlock | ||
sizeBlock:(IGListSingleSectionCellSizeBlock)sizeBlock { | ||
IGParameterAssert(identifier != nil); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's assert identifier.length > 0
IGParameterAssert(identifier != nil); | ||
IGParameterAssert(configureBlock != nil); | ||
if (self = [super init]) { | ||
_identifier = identifier; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_identifier = [identifier copy];
configureBlock:(IGListSingleSectionCellConfigureBlock)configureBlock | ||
sizeBlock:(IGListSingleSectionCellSizeBlock)sizeBlock { | ||
IGParameterAssert(identifier != nil); | ||
IGParameterAssert(configureBlock != nil); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need an assert for sizeBlock
too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rnystrom What about the other two init funcions initWithCellClass:
& initWithNibName:
? Do you want me to add assertions for sizeBlock to them as well?
} else if ([self.identifier length] > 0) { | ||
cell = [collectionContext dequeueReusableCellFromStoryboardWithIdentifier:self.identifier | ||
forSectionController:self | ||
atIndex:index]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: colon alignment
// | ||
// Created by Bofei Zhu on 10/20/16. | ||
// Copyright © 2016 Instagram. All rights reserved. | ||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs our copyright comments
// | ||
// Created by Bofei Zhu on 10/20/16. | ||
// Copyright © 2016 Instagram. All rights reserved. | ||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++copyright comments
// | ||
// Created by Bofei Zhu on 10/20/16. | ||
// Copyright © 2016 Instagram. All rights reserved. | ||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
#import "IGTestStoryboardCell.h" | ||
|
||
@implementation IGTestStoryboardCell | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably need to init the label at least? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
*/ | ||
- (__kindof UICollectionViewCell *)dequeueReusableCellFromStoryboardWithIdentifier:(NSString *)identifier | ||
forSectionController:(IGListSectionController <IGListSectionType> *)sectionController | ||
atIndex:(NSInteger)index; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this added to the .h
file, but not IGListCollectionContext.m
... am I missing something?? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jessesquires There seems to be no implementation forIGListCollectionContext
. 😶
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup just a protocol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol. derp.
implemented in IGListAdapter
🙃
Also need to update the |
@zhubofei updated the pull request - view changes |
@zhubofei updated the pull request - view changes |
@zhubofei updated the pull request - view changes |
@zhubofei updated the pull request - view changes |
@zhubofei updated the pull request - view changes |
@zhubofei updated the pull request - view changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great PR. 👍
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible we can encapsulate those two branches in a single method, e.g. controllerFromStorybard(withIdentifier identifier: String)
or pushToViewControllerFromStoryboard(withIdentifier identifier: String)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or shall we add a VC identifier property to the DemoItem class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JakeLin I added a nullable controllerIdentifier property to DemoItem class. I think it looks much neater now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++ great ideas here
override func viewDidLoad() { | ||
super.viewDidLoad() | ||
|
||
view.addSubview(collectionView) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we have storyboard, do we need to addSubview
here?
|
||
override func viewDidLayoutSubviews() { | ||
super.viewDidLayoutSubviews() | ||
collectionView.frame = view.bounds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we use Storyboard, can we use Auto Layout or autoresizing? Then we don't need this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JakeLin Nevermind. I find out I set a wrong constraint😂. Will definitely remove this line.
} | ||
|
||
let sizeBlock = { (context: IGListCollectionContext?) -> CGSize in | ||
guard let context = context else { return CGSize() } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make it more specific, I think return .zero
is better.
#import "IGTestStoryboardCell.h" | ||
|
||
@implementation IGTestStoryboardCell | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@zhubofei updated the pull request - view changes |
@zhubofei updated the pull request - view changes |
@zhubofei updated the pull request - view changes |
@zhubofei I like the idea to have |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
init( | ||
name: String, | ||
controllerClass: UIViewController.Type | ||
controllerClass: UIViewController.Type, | ||
controllerIdentifier: String?=nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ultra nit: spaces String? = nil
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) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome
|
||
func emptyView(for listAdapter: IGListAdapter) -> UIView? { | ||
return nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style nit: other VC uses { return nil }
atIndex:(NSInteger)index { | ||
IGAssertMainThread(); | ||
IGParameterAssert(sectionController != nil); | ||
IGParameterAssert(identifier != nil); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be IGParameterAssert(identifier.length > 0);
@rnystrom has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
@zhubofei updated the pull request - view changes - changes since last import |
Changes Unknown when pulling df586a1 on zhubofei:storyboard into * on Instagram:master*. |
AWESOME! |
🎉 |
Great PR to unlock Storyboard |
Changes in this pull request
This PR is for #39
Pull request checklist