[NT-889] (1/3) Landing page ui #1080
Conversation
| ) | ||
| .observeForUI() | ||
| .map { | ||
| LandingPageViewController() |
There was a problem hiding this comment.
This is temporary and will be changed once the experiment and the "first session" logic are implemented (in a different PR).
There was a problem hiding this comment.
I think as far as possible we shouldn't emit UI objects from our view models. I know this has been done previously in this VM and elsewhere but it seems like the wrong approach to me and has the potential to leak VCs and views in memory. For example, some time ago, I refactored this in RootViewModel to emit an enum instead of the actual VC - #623.
| @@ -134,6 +134,9 @@ public protocol AppDelegateViewModelOutputs { | |||
| /// Emits when the root view controller should navigate to the creator dashboard. | |||
| var goToDiscovery: Signal<DiscoveryParams?, Never> { get } | |||
|
|
|||
| /// Emits when the root view controller should present the Landing Page for new users. | |||
| var goToLandingPage: Signal<LandingPageViewController, Never> { get } | |||
There was a problem hiding this comment.
This can just be a Void or () signal and we can instantiate the VC outside of the VM.
| |> \.textColor .~ UIColor.ksr_text_dark_grey_500 | ||
| |> \.font .~ UIFont.ksr_callout() | ||
| |> \.textAlignment .~ .center | ||
| |> \.text %~ { _ in Strings.Pledge_to_projects_and_view_all_your_saved_and_backed_projects_in_one_place() |
There was a problem hiding this comment.
Can move Strings.Pledge... onto the next line to match the braces.
| internal final class LandingPageViewModelTests: TestCase { | ||
| fileprivate let vm: LandingPageViewModelType = LandingPageViewModel() | ||
|
|
||
| fileprivate let landingPageCards = TestObserver<[UIView], Never>() |
There was a problem hiding this comment.
I also don't think we should emit actual UIViews here, rather the data that will be used to configure the views or an enum type representing them.
There was a problem hiding this comment.
Yes. sorry, I forgot to mention that this was also temporary. On the second PR (containing the cards, the signal emits [LandingPageCardType]). So for now, I will delete the property from the view model test 👍
| self.labelsStackView, | ||
| self.cardsStackView, | ||
| spacer2, | ||
| spacer3, |
There was a problem hiding this comment.
Why the two spacers after each other? Are these just views that need to stretch?
There was a problem hiding this comment.
The distribution of views of this layout is a bit different of what we usually do in the app (extra spacing on top and bigger space between cards and the button). I tried configuring the views in different ways, but the result was never satisfactory. I'm open to retry if you have strong feelings about it.
There was a problem hiding this comment.
Oh I guess I'm just not sure what the two spacers adjacent to each other would do that a single spacer view can't do? How are the spacers receiving their height?
There was a problem hiding this comment.
Without the other spacer, the top view would be too low on iPhone 5s. The heights are being calculated by the stackView
| self.viewModel.outputs.goToLandingPage | ||
| .observeForUI() | ||
| .observeValues { [weak self] landingPage in | ||
| self?.rootTabBarController?.present(landingPage, animated: true) |
There was a problem hiding this comment.
@Scollaco I think for the landing page we want it to be presented as a full screen modal - on iOS 13 the automatic modalPresentationStyle is pageSheet, so you'll want to be specific and set it to fullScreen so that it presents as full screen on iOS 13.
Generated by 🚫 Danger |
|
|
||
| private enum Layout { | ||
| enum Button { | ||
| static let height: CGFloat = 54 |
There was a problem hiding this comment.
Any reason we're not using our Styles.minTouchSize.height for buttons here?
| } | ||
|
|
||
| public final class LandingPageViewController: UIViewController { | ||
| // Properties |
| ]) | ||
| } | ||
|
|
||
| @objc private func ctaBUttonTapped() { |
There was a problem hiding this comment.
Typo here, should be ctaButtonTapped
| label | ||
| |> \.text %~ { _ in Strings.Bring_creative_projects_to_life() } | ||
| |> \.font .~ UIFont.ksr_title3().bolded | ||
| |> \.textAlignment .~ .center |
There was a problem hiding this comment.
Do we want to this to wrap with larger font sizes? Right now it's truncating.
📲 What
. This is the first of a 3 parts work to create our new LandingPage.
🤔 Why
We want to tell new users a little more about Kickstarter during their first session.
JIRA ticket
🛠 How
Note: There's no logic to present the viewController yet. This will be done in a different PR. So, for now, the viewController will be presented every time the app launches.
👀 See
♿️ Accessibility
✅ Acceptance criteria