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.

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

Close Button Missing #102

Closed
mbbischoff opened this issue Dec 15, 2015 · 8 comments
Closed

Close Button Missing #102

mbbischoff opened this issue Dec 15, 2015 · 8 comments
Labels

Comments

@mbbischoff
Copy link
Contributor

Steps to Reproduce

  1. Run either of the sample projects.
  2. Tap the photo.

Expected Results

An “X” button called NYTPhotoViewerCloseButtonX is displayed in the upper left corner of the view controller.

Actual Results

No “X” button is displayed and the nyt_photoViewerResourceBundle method returns nil.

Platform and OS

Tested both Swift 2 (with minor changes to get it to compile) and Objective-C sample projects as well as another project.

Xcode Version 7.2 (7C68)

@cdzombak
Copy link
Contributor

cdzombak commented Jan 5, 2016

We're looking into this. @Twigz isn't able to repro this issue right now, though I am. This is weird because this definitely did work for me when I was testing #80.

@cdzombak
Copy link
Contributor

cdzombak commented Jan 5, 2016

One avenue for investigation: when this worked, the pod was installed in the Example project with CocoaPods 0.38.2 ( d27f3d2#diff-801cd3156cf43adeb08ac4958dccb4d7 ). The Example project is now broken, on CocoaPods 0.39.0 ( https://github.com/NYTimes/NYTPhotoViewer/blob/develop/Example/Podfile.lock ).

Reviewing the CocoaPods changelog between those versions, CocoaPods/CocoaPods#4065 is the only change between those versions that jumps out to me as possibly-related.

@bcapps
Copy link
Contributor

bcapps commented Jan 7, 2016

I'm still seeing this issue. It can be observed in the sample project by adding use_frameworks! to the Podfile, running pod install with CocoaPods 0.39.0, and running the Swift target. I'm not sure which of those steps is the one that does it, but I've got the code on the branch feature/swift-framework.

@bcapps bcapps reopened this Jan 7, 2016
@cdzombak
Copy link
Contributor

cdzombak commented Jan 7, 2016

Without any actual experimenting, I would bet that use_frameworks! somehow makes some difference with resource bundles.

@cdzombak
Copy link
Contributor

cdzombak commented Jan 7, 2016

Do we have to use resource bundles for this? I suspect building a resource bundle as part of the Cocoapods install process will be a problem for Carthage support #61/#63.

I would be totally fine with moving to some other system and ignoring whatever annoying warning Cocoapods gives us about not using resource_bundles. Based on our experience so far they're nothing but unreliable trouble, and I've found nearly no good documentation about how we're expected to use them, and I'm tired of this.

@bcapps
Copy link
Contributor

bcapps commented Jan 7, 2016

@cdzombak Using bundleForClass instead of mainBundle to retrieve the resources bundle fixed the issue for me on this same branch. Check commit 47b6240e430282f0a0358b7034b35792cdfaad52 to see it working. I haven't tested this change without frameworks or in Objective-C, but the results are preliminarily promising anyway.

Additionally, it might be a good idea to update the bundle calls to use the URL methods instead of the stringly-typed path ones. I did that, too, in another commit.

@cdzombak
Copy link
Contributor

cdzombak commented Jan 7, 2016

Using bundleForClass instead of mainBundle to retrieve the resources bundle fixed the issue for me on this same branch.

Huh, that definitely makes sense, and that would explain why use_frameworks results in different behavior.

Additionally, it might be a good idea to update the bundle calls to use the URL methods instead of the stringly-typed path ones.

Should make no difference whatsoever, but 👍

@liruqi
Copy link

liruqi commented Jan 10, 2016

Cannot remove use_frameworks! in Swift project.
Changing Podfile could fix this issue, replace pod 'NYTPhotoViewer' with
pod 'NYTPhotoViewer', :git => 'https://github.com/NYTimes/NYTPhotoViewer', :branch => 'feature/close-button'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants