modal blocked #84
modal blocked #84
Conversation
|
@paulmiard @MichaelCampbell I couldn't add you as a reviewer on the PR, so if you can add it to your list of pending reviews to check it when you have a chance it would be great. Thanks on advance! |
There was a problem hiding this comment.
This looks great, thanks for working on it @juangdelvalle. Sorry for the delay on reviewing.
I left some inline comments, but I think we should be good to merge once the travis build passes. Let me know if you need some support getting it to pass.
Yoshi.podspec
Outdated
| s.description = <<-DESC | ||
| Yoshi is a convenient wrapper around the UI code that is often needed for displaying debug menus. Out of the box, Yoshi provides easy-to-implement date, list and custom menus. | ||
| DESC |
There was a problem hiding this comment.
Is the description no longer required — curious why you removed it?
There was a problem hiding this comment.
it was by mistake, I'll add it back right away
| s.author = { "Michael Campbell" => "[email protected]" , "Luna An" => "[email protected]"} | ||
| s.source = { :git => "https://github.com/prolificinteractive/Yoshi.git", :tag => s.version.to_s } | ||
|
|
||
| s.platform = :ios, '8.0' |
There was a problem hiding this comment.
Is platform no longer required?
There was a problem hiding this comment.
platform is now specified in s.ios.deployment_target = '8.0'
| s.name = 'Yoshi' | ||
| s.version = '3.0.1' | ||
| s.summary = 'A helpful companion for your iOS app.' | ||
| s.license = 'MIT' |
There was a problem hiding this comment.
Should we no longer link reference the license file?
There was a problem hiding this comment.
as I understand, is no longer required to make a reference to a license file, but I can change it to make the reference anyway.
Yoshi.podspec
Outdated
| s.screenshots = 'https://raw.githubusercontent.com/prolificinteractive/Yoshi/a6e85e87cbd67f2bb3bfe60157e7b13281d80f20/Images/Yoshi.png", "https://raw.githubusercontent.com/prolificinteractive/Yoshi/c66cdf8dc2ab643fe57996d20d3cd37b8b70ceff/Images/Yoshi_iPad.png' | ||
|
|
||
| s.ios.deployment_target = '8.0' | ||
| s.swift_versions = ['4.2', '5.0', '5.1'] |
There was a problem hiding this comment.
There is an open PR that takes care of Swift 5 changes. Don't we need to merge it before adding 5.x to the list?
See #83
There was a problem hiding this comment.
In this PR I also migrated the code to Swift 5 (the other PR wasn't approved yet so I thought about killing two birds with the same stone)
There was a problem hiding this comment.
I didn't look at the details in the other PR but looked like there was more going on. Are you saying the other PR can be declined all together once this is merged? I guess I will diff it to see what else was in there once we merge but happy to hear your thoughts
@MichaelCampbell I might need help to get it to pass, the error that I'm getting is: |
I took a look into this framework and it seems that they're doing something similar to us: https://github.com/DianQK/TransitionTreasury/blob/master/.travis.yml Maybe try bumping the pod version to latest @juangdelvalle |
|
Hi @juangdelvalle, any progress on this PR? |
Set a presentationController delegate to be able to capture the event when the presented view is dismissed by a swap down with the new modality management of iOS 13, implementing the presentationControllerDidDismiss method