-
Notifications
You must be signed in to change notification settings - Fork 2
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
Amanda/ui features #30
base: pheonix
Are you sure you want to change the base?
Conversation
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.
We'll get you higher resolution images today at the meeting, so I'm going to withhold approval for now until these changes are made.
KDIC/PlayerViewController.swift
Outdated
@@ -7,16 +7,22 @@ class PlayerViewController: UIViewController { | |||
@IBOutlet weak var subtitleLabel: UILabel! | |||
@IBOutlet weak var imageView: UIImageView! | |||
|
|||
@IBOutlet weak var KDICPhoto: UIImageView! |
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 think this is the same as imageView
.
KDIC/PlayerViewController.swift
Outdated
override func viewDidAppear(_ animated: Bool) { | ||
super.viewDidAppear(animated) | ||
print("About to play") | ||
KDICPlayer.play() | ||
print("Playing!") | ||
} | ||
|
||
|
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: Extraneous newline
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.
Please fix.
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.
Done.
KDICCore/KDICPlayer.swift
Outdated
@@ -20,6 +20,7 @@ open class KDICPlayer { | |||
player.pause() | |||
} | |||
|
|||
/** to fix **/ |
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.
We've made a ticket, so you can remove this comment.
Alternatively, if you'd like to keep it, we should use proper Swift documentation syntax. You can find information on that here. (Hint: There is a tag called Bug
.)
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 you choose to do the documentation, make sure you reference the github issue number in the comment (#29).
Also, make sure that you submit pull requests which merge into the |
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.
Please also remove the Screen Shot file from the repository (git rm --cached KDIC/Base.lproj/Screen\ Shot\ 2017-03-08\ at\ 5.54.02\ PM.png
). Otherwise this looks really great!
KDIC/PlayerViewController.swift
Outdated
override func viewDidAppear(_ animated: Bool) { | ||
super.viewDidAppear(animated) | ||
print("About to play") | ||
KDICPlayer.play() | ||
print("Playing!") | ||
} | ||
|
||
|
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.
Please fix.
KDICCore/KDICPlayer.swift
Outdated
open class KDICPlayer { | ||
private static var player: AVPlayer = { | ||
let asset = AVURLAsset(url: streamURL) | ||
let playerItem = AVPlayerItem(asset: asset) | ||
// |
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.
Please remove
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.
Done
KDICCore/KDICPlayer.swift
Outdated
} | ||
|
||
/** to fix **/ |
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.
Please remove.
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.
Done
KDICCore/KDICPlayer.swift
Outdated
open class func toggle() { | ||
if isPlaying { | ||
if isPlaying == 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.
Must we use ==
here? Can't we just do if isPlaying
?
KDICCore/KDICPlayer.swift
Outdated
@@ -3,29 +3,40 @@ import AVFoundation | |||
|
|||
private let streamURL = URL(string: "http://kdic.grinnell.edu/stream")! | |||
|
|||
let kdicInstance = KDICPlayer(); | |||
|
|||
private var isPlaying = false; |
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.
Try private(set) var isPlaying = false
This will resolve my comment below.
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.
done
KDICCore/KDICPlayer.swift
Outdated
pause() | ||
} else { | ||
play() | ||
} | ||
} | ||
|
||
open func currentlyPlaying() -> Bool { |
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.
Use a semi-public property above to make a private setter with a public getter. I put some code above. Please then remove this function.
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.
done(?) Please check this one real quick
KDICCore/KDICPlayer.swift
Outdated
@@ -3,29 +3,40 @@ import AVFoundation | |||
|
|||
private let streamURL = URL(string: "http://kdic.grinnell.edu/stream")! | |||
|
|||
let kdicInstance = KDICPlayer(); |
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.
Can we just call this variable player
?
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.
Actually, do you ever use this variable? I don't think we need it.
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.
done, you're right I don't even use it lol
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.
Please also use the git command I posted earlier to remove the screenshot.
} | ||
|
||
open class func toggle() { | ||
if isPlaying { | ||
if isPlaying == 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.
Do we need to have == true
here? Please remove if it compiles without it. isPlaying
is a Bool
.
KDICCore/KDICPlayer.swift
Outdated
open class KDICPlayer { | ||
private static var player: AVPlayer = { | ||
let asset = AVURLAsset(url: streamURL) | ||
let playerItem = AVPlayerItem(asset: asset) | ||
let currentlyPlaying = isPlaying; |
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.
You never use this binding.
KDICCore/KDICPlayer.swift
Outdated
@@ -3,25 +3,28 @@ import AVFoundation | |||
|
|||
private let streamURL = URL(string: "http://kdic.grinnell.edu/stream")! | |||
|
|||
private(set) var isPlaying = false; |
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 in the class, like on line 9.
KDIC/PlayerViewController.swift
Outdated
override func viewDidAppear(_ animated: Bool) { | ||
super.viewDidAppear(animated) | ||
print("About to play") | ||
KDICPlayer.play() | ||
print("Playing!") | ||
} | ||
|
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: Extraneous newline.
Hi -- just pulled latest and hit this issue... Please ignore if this is known and/or already in progress: "Type ANY has no subscript members..." File: FeedViewController.swift
=== Thanks |
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.
KDIC/Base.lproj/Screen Shot 2017-03-08 at 5.54.02 PM.png
still needs to be removed
KDIC/PlayerViewController.swift
Outdated
|
||
self.imageView.image = UIImage(named: "Equalizer") | ||
|
||
button.setImage(imgShell1, forState:.Normal); |
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.
Does this work? Where is imgShell1
coming from? Same for coin
in the next line.
KDIC/PlayerViewController.swift
Outdated
override func viewDidAppear(_ animated: Bool) { | ||
super.viewDidAppear(animated) | ||
print("About to play") | ||
KDICPlayer.play() | ||
print("Playing!") | ||
} | ||
|
||
func resizeImageWithAspect(image: UIImage,size: CGSize)->UIImage |
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 don't think you need this. You can use the Aspect Fill
setting I showed you on the storyboard to adjust how the image appears in the UIImage
view.
How to solve this? File: FeedViewController.swift thanks, func loadTableData(_ day: String){
} |
Added KDIC image. Closes #29