Skip to content
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

[NT-892, NT-893] (2/3) Landing page carousel #1083

Merged
merged 12 commits into from
Feb 28, 2020

Conversation

Scollaco
Copy link
Contributor

📲 What

. This is the second part of a 3 parts work to create our new LandingPage.
. It adds the stats and how to cards to the Landing Page screen.

🤔 Why

We want to tell new users a little more about Kickstarter during their first session.

JIRA tickets:
Ticket 1
Ticket 2

🛠 How

  • Created LandingPageCardType with respective values
  • Created LandingPageStatsView and its view models
  • Added Strings
  • Added experiment to return card types based on the variants
  • Added tests

👀 See

Trello, screenshots, external resources?

Before 🐛 After 🦋
Simulator Screen Shot - iPhone 11 Pro - 2020-02-24 at 11 48 56 Simulator Screen Shot - iPhone 11 Pro - 2020-02-24 at 17 39 41

♿️ Accessibility

  • Works with VoiceOver
  • Supports Dynamic Type

✅ Acceptance criteria

  • The layout matched the design from Figma.

Copy link
Contributor

@ifbarrera ifbarrera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple visual notes:

  1. The title label font for the stats cards looks a little small compared to the designs
    image

  2. Also I noticed that kept the horizontal scroll bar - should we consider removing it, or is this intentional?

@@ -61,6 +62,7 @@ public final class LandingPageViewController: UIViewController {
.observeForUI()
.observeValues { [weak self] cards in
self?.configureCards(with: cards)
self?.updatePageControl(with: cards)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this should be a separate output pageControlCount so it can be tested independently 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, good idea! 👍

public var quantity: String? {
switch self {
case .successfulProjects:
return Strings.Total_plus(total: "177,000")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use Format.wholeNumber here so the number is localized?

case .successfulProjects:
return Strings.Total_plus(total: "177,000")
case .totalBackers:
return Strings.Million_plus(total_amount: "17")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here though this one is probably fine.

@nativeksr
Copy link
Collaborator

nativeksr commented Feb 26, 2020

1 Warning
⚠️ Big PR

Generated by 🚫 Danger

@Scollaco Scollaco merged commit 3748d2a into NT-889-landing-page-UI Feb 28, 2020
@Scollaco Scollaco deleted the NT-892-landing-page-carousel branch February 28, 2020 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants