[wip stretch goal] Live stream discovery #53

mbrandonw · 2017-01-20T07:41:00Z

Hey everyone, Justin and I wanted to get one more thing done before my time in cape town is over (sniff), and so we thought about adding a lil bit of discovery to the live streams. Currently you can only find live streams if you go straight to a project, no deep-linking or push notifications yet :/

So, we just added a lil "Kickstarter Live" entry to the discovery filters:

Tapping that takes you to a feed of live, upcoming and recent live streams:

Tapping those cards either goes to the countdown, live stream or reply.

NB

This PR has a few things that I wanna call out:

We are now hitting two api endpoints on the live api: one to get a list of events and one to get a single event. these two endpoints return subtly different json unfortunately and so i had to do a bit of gymnastics to accommodate for this. i'll call out the lines in my self-review.
Due to the above, i had to change the live stream models quite a bit, which then wreaked havoc on the lenses. So, I wanted to try something new to ease the creation of lenses for us (at least until we get code gen). If we set all of the fields of a struct to public private(set) var field, we still get to act like we are immutable to the outside world, but inside this file only we can make lenses like:

extension Model {
  public enum lens {
    public static let field = Lens<Model, Int>(
      view: { $0.field },
      set: { var new = $1; new.field= $0; return field; }
    )
  }
}

not only is it less code, but it doesnt break when we add new fields. whatdya'll think?

…i endpoint.

mbrandonw · 2017-01-20T07:54:09Z

Kickstarter-iOS/DataSources/LiveStreamDiscoveryDataSource.swift

+  }
+}
+
+private func sorted(liveStreamEvents: [LiveStreamEvent]) -> [LiveStreamEvent] {


if the code in this function looks suspiciously like one i cooked up in #45, then you are right. unfortunately i cant share the code cause one operates on LiveStreamEvents and the other on Project.LiveStream :(((

the models are a bit messy due to hitting two different apis right now. we are gonna try to find a way to bridge these two worlds soon.

actually never mind on this! with @justinswart's recent refactoring work to get rid of Project.LiveStream #63 i was able to share all of this code.

mbrandonw · 2017-01-20T07:56:40Z

Kickstarter-iOS/DataSources/LiveStreamDiscoveryDataSource.swift

+        events.forEach { event in
+          self.appendRow(value: event, cellClass: LiveStreamDiscoveryCell.self, toSection: title.section)
+        }
+    }


^^^ this is a fun lil thing. it does the work to create titles for live/upcoming/recent with live streams grouped into them. it's heavily tested too...

cool! just thinking, we coullllld in the future, maybe, update Sorts to have these 3 categories.... if this feature becomes really popular, it might be nice to go directly to these sections. Could have a sort option for streams your friends subscribe too..

@theginbin ^^^ that would be awesome! a really nice lil step after the initial release.

mbrandonw · 2017-01-20T07:57:25Z

Kickstarter-iOS/Tests/Views/LiveStreamContainerViewControllerTests.swift

@@ -23,11 +23,11 @@ internal final class LiveStreamContainerViewControllerTests: TestCase {
      |> Project.LiveStream.lens.startDate .~ (MockDate().timeIntervalSince1970 - 86_400)
      |> Project.LiveStream.lens.isLiveNow .~ false
    let liveStreamEvent = .template
-      |> LiveStreamEvent.lens.stream.hasReplay .~ true


I got rid of the Stream sub-struct of LiveStreamEvent in order to flatten the models a bit.

mbrandonw · 2017-01-20T08:02:46Z

Kickstarter-iOS/Views/Controllers/LiveStreamCountdownViewController.swift

@@ -377,32 +379,3 @@ public final class LiveStreamCountdownViewController: UIViewController {
    self.eventDetailsViewModel.inputs.subscribeButtonTapped()
  }
 }
-
-// Returns a fancy monospaced font for the countdown.
-private func countdownFont(label: UILabel) -> UIFont {


we use this twice now so i extracted it to a helper on UIFont, can just do baseFont.countdownMonospaced

mbrandonw · 2017-01-20T08:04:31Z

Kickstarter-iOS/Views/Controllers/LiveStreamDiscoveryViewController.swift

+    nav.viewControllers = [vc]
+    DispatchQueue.main.async {
+      self.present(nav, animated: true, completion: nil)
+    }


we were seeing some weird flickering of the screen when not using DispatchQueue.main.async :/ it seems to be entirely due to initializing the nav controller and then setting its controllers, as opposed to using the UINavigationController(rootViewController:) initializer.

mbrandonw · 2017-01-20T08:05:38Z

Library/SharedFunctions.swift

+///
+/// - returns: A signal producer.
+public func countdownProducer(to date: Date)
+  -> SignalProducer<(day: String, hour: String, minute: String, second: String), NoError> {


this is a fun function! we use it in the countdown controller and the countdown of the live stream discovery cells.

lots of tests here: https://github.com/kickstarter/ios-oss/pull/53/files#diff-c7a229a8e1612db2ccad1308b69894a5

mbrandonw · 2017-01-20T08:12:45Z

Library/ViewModels/DiscoveryNavigationHeaderViewModel.swift

@@ -297,6 +297,8 @@ private func stringsForTitle(params: DiscoveryParams) -> (filter: String, subcat

  if params.staffPicks == true {
    filterText = Strings.Projects_We_Love()
+  } else if params.hasLiveStreams == .some(true) {
+    filterText = "Kickstarter Live"


do we need to localize this? or is this a branding thing? /cc @ktman @rreymundi

Hey @awakinglim mind weighing in here? Curious what you think from brand, UX, localization perspective? This will appear in the discovery overlay and let you access cards for upcoming, live, and recent streams.

mbrandonw · 2017-01-20T08:15:25Z

Library/ViewModels/DiscoveryViewModel.swift

+  var discoveryPagesViewHidden: Signal<Bool, NoError> { get }
+
+  /// Emits a boolean that determines if the live stream discovery view is hidden.
+  var liveStreamDiscoveryViewHidden: Signal<Bool, NoError> { get }


so the livestream discovery controller is an embedded controller in DiscoveryViewController, which means the discovery pages are its siblings. this vm will now take care of hiding and showing them appropriately.

mbrandonw · 2017-01-20T08:15:59Z

Library/ViewModels/DiscoveryViewModel.swift

+    let currentParams = Signal.merge(
+      self.viewWillAppearProperty.signal.take(first: 1).mapConst(DiscoveryViewModel.defaultParams),
+      self.filterWithParamsProperty.signal.skipNil()
+      )


just a small refactor while i was here. don't know why i jumped through hoops for that weird producer thing when it can be done so much more easily...

mbrandonw · 2017-02-07T19:26:32Z

Kickstarter-iOS/Views/Controllers/LiveStreamCountdownViewController.swift


    _ = self.hoursSubtitleLabel
-      |> UILabel.lens.text %~ { _ in localizedString(key: "days", defaultValue: "hours") }
+      |> UILabel.lens.text %~ { _ in Strings.hours() }


we had a typo here! we were using days for hours! 😱

😳 good catch!

justinswart

Just a couple observations!

Great work! This is going to be great 👍

justinswart · 2017-02-07T19:26:59Z

Kickstarter-iOS/Locales/Base.lproj/Localizable.strings

@@ -49,6 +49,7 @@
 "Continue_to_payment" = "Continue to payment";
 "Continue_to_update_pledge" = "Continue to update pledge";
 "Couldnt_add_attachment" = "Couldn't add attachment";
+"Couldnt_open_live_stream_Try_again_later" = "Couldn‘t open live stream. Try again later.";


Is that a backtick in Couldn't? Doesn't look like a normal apostrophe.

it's a "smart" apostrophe http://smartquotesforsmartpeople.com

that site is a bit much :/

justinswart · 2017-02-07T19:28:12Z

Kickstarter-iOS/Tests/DataSources/LiveStreamDiscoveryDataSourceTests.swift

+@testable import Kickstarter_Framework
+@testable import Library
+@testable import LiveStream
+@testable import KsApi


Could alphabetize

justinswart · 2017-02-07T19:30:15Z

Kickstarter-iOS/Tests/Views/LiveStreamCountdownViewControllerTests.swift

+    parent.view.setNeedsLayout()
+    vc.view.setNeedsLayout()
+    parent.view.setNeedsUpdateConstraints()
+    vc.view.setNeedsUpdateConstraints()


What were all the layout passes needed for?

i cant remember! lemme try without and see what happens

not needed at all! removing!

justinswart · 2017-02-07T19:53:50Z

Library/Tests/SharedFunctionsTests.swift

+  func testCountdownProducer() {
+    let future: TimeInterval = TimeInterval(1*60*60*24) + TimeInterval(16*60*60) + TimeInterval(34*60) + 2
+    let futureDate = MockDate().addingTimeInterval(future).date
+    let countdown = countdownProducer(to: futureDate)


So DRY 😄

brave new world for me!

justinswart · 2017-02-07T19:57:19Z

Library/Tests/ViewModels/LiveStreamDiscoveryLiveNowCellViewModelTests.swift

+@testable import KsApi
+@testable import Library
+@testable import LiveStream
+import Prelude


Alphabetize

justinswart · 2017-02-07T19:58:26Z

Library/Tests/ViewModels/LiveStreamDiscoveryViewModelTests.swift

+@testable import KsApi
+@testable import Library
+@testable import LiveStream
+import Prelude


justinswart · 2017-02-07T20:01:06Z

Library/ViewModels/LiveStreamContainerViewModel.swift

@@ -91,7 +101,7 @@ LiveStreamContainerViewModelInputs, LiveStreamContainerViewModelOutputs {
      )
      .map(first)

-    let initialEvent = configData.map(second)
+    let initialEvent = configData.map { _, event, _, _ in event }


We should add a fourth sometime 😄

we really should!

justinswart · 2017-02-07T20:02:20Z

Library/ViewModels/LiveStreamCountdownViewModel.swift

-      }
-      .map { (day: $0.day ?? 0, hour: $0.hour ?? 0, minute: $0.minute ?? 0, second: $0.second ?? 0) }
+      .take(first: 1)
+      .switchMap { countdownProducer(to: $0.startDate) }


justinswart · 2017-02-07T20:06:54Z

LiveStream/Models/LiveStreamEvent.swift

@@ -64,6 +64,40 @@ public struct LiveStreamEvent: Equatable {
  public var definitelyHasReplay: Bool {
    return self.hasReplay && self.replayUrl != nil
  }
+
+  public static func canonicalLiveStreamEventComparator(now: Date) -> Prelude.Comparator<LiveStreamEvent> {


justinswart

👍

mbrandonw added 21 commits January 19, 2017 08:28

made some stuff optional so that we could hit the live stream list ap…

7b742dd

…i endpoint.

Flattened live stream event, and consolidated jsonn decoding.

da3620b

updated library and app target for new live stream event data.

d183605

lots of work on live stream discover

565e287

some clean up

fd17aef

swiftlint

6d43625

lots of work on live stream discovery

8da88af

bad way of opening live stream from discovery

54d0bfe

clean up

c5518b0

swiftlint

8549988

fixed a bunch of tests

2d9cd81

better datasource and now tests.

4002b90

wrapped some strings

add5ecb

swiftlint and one more test

284a1ea

swiftlint

576e73e

lots of clean up

7bc9780

added green dot to discover filters

f7c74bd

update secrets

11e4b50

clean up

7c291f1

small fix

becdd27

Merge remote-tracking branch 'oss/master' into live-stream-disc

6fd98ee

mbrandonw commented Jan 20, 2017

View reviewed changes

mbrandonw and others added 18 commits February 4, 2017 12:14

swiftlint

6b616fd

fixed api param encoding

1f60267

lots of work

39d0167

fixed tests and updated screenshots

a8f016d

swiftlint

f697ab0

swiftlint

80b3163

lots of styling and testing

576947c

Merge remote-tracking branch 'oss/master' into live-stream-disc

6192db2

shared some live stream styles.

f969a57

swiftlint

b0723c3

Fix missing string build issue

1cd39bf

Merge remote-tracking branch 'oss/master' into live-stream-disc

6c5e917

more design updates

846f17b

swiftlint

bf225c9

update ksapi

b419c54

update lots of strings

cf76913

swiftlint

1c0075e

extracted some styles

88bf31a

mbrandonw commented Feb 7, 2017

View reviewed changes

last minute clean up

661eff6

justinswart reviewed Feb 7, 2017

View reviewed changes

justinswart self-requested a review February 7, 2017 20:14

justinswart approved these changes Feb 7, 2017

View reviewed changes

mbrandonw added 4 commits February 7, 2017 15:27

addressed some feedback

67b9fc4

Merge remote-tracking branch 'oss/master' into live-stream-disc

7d4ce20

separate feature flag for live stream discovery

598c3f1

swiftlint

c03a84c

mbrandonw merged commit 6837f3f into master Feb 7, 2017

mbrandonw deleted the live-stream-disc branch February 7, 2017 21:31

justinswart mentioned this pull request Apr 26, 2017

Possible fix for setViewController crash #139

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wip stretch goal] Live stream discovery #53

[wip stretch goal] Live stream discovery #53

mbrandonw commented Jan 20, 2017

mbrandonw Jan 20, 2017

mbrandonw Feb 1, 2017

mbrandonw Jan 20, 2017

theginbin Jan 23, 2017

mbrandonw Jan 31, 2017

mbrandonw Jan 20, 2017

mbrandonw Jan 20, 2017

mbrandonw Jan 20, 2017 •

edited

mbrandonw Jan 20, 2017

mbrandonw Jan 20, 2017

ktman Jan 20, 2017

mbrandonw Jan 20, 2017

mbrandonw Jan 20, 2017

mbrandonw Feb 7, 2017

justinswart Feb 7, 2017

justinswart left a comment

justinswart Feb 7, 2017

mbrandonw Feb 7, 2017

justinswart Feb 7, 2017

justinswart Feb 7, 2017

mbrandonw Feb 7, 2017

mbrandonw Feb 7, 2017

justinswart Feb 7, 2017

mbrandonw Feb 7, 2017

justinswart Feb 7, 2017

justinswart Feb 7, 2017

justinswart Feb 7, 2017

mbrandonw Feb 7, 2017

justinswart Feb 7, 2017

justinswart Feb 7, 2017

justinswart left a comment

[wip stretch goal] Live stream discovery #53

[wip stretch goal] Live stream discovery #53

Conversation

mbrandonw commented Jan 20, 2017

NB

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbrandonw Jan 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinswart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinswart left a comment

Choose a reason for hiding this comment

mbrandonw Jan 20, 2017 •

edited