[HACK WEEK] Example Swift App with UI and Unit Test targets [SWIFT 1.1, Xcode 6.2 rel] #27
Conversation
|
Hey @bcapps @Twigz , @cdzombak @mliberatore @jasonhawkins @yorkepb I've downgraded this NYTPhotoViewer example app and unit tests to support Swift 1.1 from Xcode 6.2. I'll be keen to hear any comments or suggestions you have. |
| var placeholderImage: UIImage? | ||
| var attributedCaptionTitle: NSAttributedString | ||
| var attributedCaptionSummary: NSAttributedString | ||
| var attributedCaptionCredit: NSAttributedString |
There was a problem hiding this comment.
Since these 5 properties are only set in the initializer, can we declare them let instead of var?
There was a problem hiding this comment.
I've reduced 5 down to 2: image and attributedCaptionTitle are still variable. The compiler won't let me assign to those two, even in the init() if they are declared as constants.
There was a problem hiding this comment.
Sorry, I was looking at a different error message. Yes we are allowed to assign an initial value during initialization to a constant property that does not already have a default property value. Dow to 1 variable that does get modified after initialization.
|
Where did the "Testing" test image come from? If we plan to release this publicly, we should ensure we have some right to use that image. |
|
|
||
| class ViewController: UIViewController, NYTPhotosViewControllerDelegate { | ||
|
|
||
| @IBOutlet weak var imageButton : UIButton! |
There was a problem hiding this comment.
Oops, I meant to change this after our conclusion on the other PR!
Done.
|
@cdzombak It's the one that was already in the project, and it comes from a Google search with the "Labeled for reuse" Usage Rights filter. Looking at it more closely, though, I can't actually find the original source and accompanying license, so Google may have been mistaken. But this brings up a good point - why was the asset catalog re-added? |
|
Aside from the stuff I called out, this looks fine to me. |
| @@ -111,8 +111,8 @@ - (NSDictionary *)photosViewController:(NYTPhotosViewController *)photosViewCont | |||
| return nil; | |||
| } | |||
|
|
|||
| - (void)photosViewController:(NYTPhotosViewController *)photosViewController didDisplayPhoto:(id <NYTPhoto>)photo { | |||
| NSLog(@"Did Display Photo: %@ identifier: %@", photo, @([self.photos indexOfObject:photo]).stringValue); | |||
| - (void)photosViewController:(NYTPhotosViewController *)photosViewController didDisplayPhoto:(id<NYTPhoto>)photo atIndex:(NSUInteger)photoIndex { | |||
|
Thanks for taking the time @cdzombak !! |
|
@bcapps I've shared the original asset catalog in a common group/folder and removed the copy. |
|
|
||
| @IBOutlet weak var imageButton : UIButton? | ||
| private let photos = PhotosProvider().photos | ||
| private var photosViewController: NYTPhotosViewController |
There was a problem hiding this comment.
The original intention of the sample project was to mimic how we thought the view controller would be used in most applications. It seems unlikely to me that apps would have a property for a view controller that they will present in the future and that they would initialize that view controller this early, in the initializer. It seems more likely, to me at least, that they would create and instantiate a view controller at the time they need to display it - in this case, when the image is tapped. The Objective-C version of the sample app instantiates the view controller without storing a property when the image is tapped, and I think that it makes a bit more sense and is closer to how people would usually use it in their apps.
There was a problem hiding this comment.
Totally agree, fixed in the next commit.
|
@cdzombak You should find your UX weirdness is fixed! |
|
Okay @bcapps, have another look |
[HACK WEEK] Example Swift App with UI and Unit Test targets [SWIFT 1.1, Xcode 6.2 rel]
|
@cdzombak Yes, that commit resolved Brian's concern and took care of all the weirdness you reported: wrong background color, wrong initial view origin, etc. It looked as though the view controller was being instantiated twice, but I haven't verified this so I could be mistaken. |
Demonstrates use of NYTPhotoViewer from an app written in Swift with UI and unit tests
NYTPhotosViewControllercauses a crash. Although the crasher will be fixed in the underlying pod, the workaround provides a simple demonstration of passing a closure that would otherwise be unnecessary.