-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Refreshing of the Live Stream Discovery view #81
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.
looks mostly good, just a few questions!
@@ -13,6 +16,9 @@ public protocol LiveStreamDiscoveryViewModelInputs { | |||
|
|||
/// Call when the view loads. | |||
func viewDidLoad() | |||
|
|||
/// Call when the viewWillAppear | |||
func viewWillAppear() |
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 viewWillAppear
not get called again when bringing to the foreground?
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.
Nope, I tried doing it with just that - turns out it's not called.
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.
good to know! #TIL
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.
👍
.flatMap { _ in | ||
let periodicRefresh = self.viewDidLoadProperty.signal.switchMap { | ||
timer(interval: .seconds(60*5), on: AppEnvironment.current.scheduler) | ||
} |
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 kinda think we don't need this countdown. it would only be useful if the person sat on this page for 5 mins. otherwise navigating away and back should trigger the view will appear.
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.
Ok, cool!
@@ -30,6 +30,14 @@ internal final class LiveStreamDiscoveryViewController: UITableViewController { | |||
internal override func bindViewModel() { | |||
super.bindViewModel() | |||
|
|||
NotificationCenter.default.addObserver( | |||
forName: NSNotification.Name.UIApplicationWillEnterForeground, object: nil, | |||
queue: nil) { [weak self ]_ in |
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.
the newline placement is a little off, could we try something like
NotificationCenter.default
.addObserver(forName: .UIApplicationWillEnterForeground, object: nil, queue: nil) { [weak self ] _ in
self?.viewModel.inputs.appWillEnterForeground()
}
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.
ah yeah that does look a little weird, cool!
This adds a couple of different ways that the Live Stream Discovery view will be refreshed to avoid navigating back and seeing 'stale' cards that are perhaps no longer live and now in replay, or were upcoming and are now live, etc.
It will refresh the following ways:
isActive
is set totrue
(causes a fetch as before)isActive
is set tofalse
(clears data source as before)All of these will only emit when
isActive
istrue
.Having done all of this I'm now no longer sure if this is really what we want, it feels like there are just so many ways that this view can be refreshed, but it's done and seems to work fine so let me know what you think!