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-505] Project page creators header #930

Merged
merged 27 commits into from
Nov 7, 2019
Merged

Conversation

Scollaco
Copy link
Contributor

@Scollaco Scollaco commented Nov 4, 2019

πŸ“² What

  • Adds a header to allow the creator to easily see dashboard/progress of the project she/he created.
    Note: This PR doesn't handle the case when contributors visit the project page. This will be done in the future.

πŸ€” Why

This task is the first part of a series of tickets that will prevent creators of backing their own project.
JIRA ticket

πŸ›  How

  • Created ProjectPamphletCreatorHeaderCell and its viewModel.
  • Created ProjectPamphletCreatorHeaderCellDelegate to notify the ProjectPamphletContentViewController when the user taps the View progress/dashboard button.
  • The top margin on the Video storyboard was a little off and it was preventing the cell to be presented correctly.
  • Added translations
  • Added tests for viewModel, dataSource, and viewController.

πŸ‘€ See

Non Creator πŸ› Creator πŸ¦‹
Simulator Screen Shot - iPhone 8 - 2019-11-04 at 16 46 44 Simulator Screen Shot - iPhone 8 - 2019-11-04 at 16 42 53

♿️ Accessibility

  • Tap targets use minimum of 44x44 pts dimensions
  • Works with VoiceOver
  • Supports Dynamic Type

βœ… Acceptance criteria

** Logged-in using the therealnativesquad account:

  • Select any non-created project from Discovey. The header should be hidden.
  • Search for a project created using that account (e.g Help me transform this pile of wood and select it. The Project page should be presented with the header containing a label indicating when the project was created and a button showing View progress if project is live or View dashboard if project is no longer live.

@@ -19,7 +19,7 @@
<autoresizingMask key="autoresizingMask" widthSizable="YES" heightSizable="YES"/>
<subviews>
<containerView opaque="NO" contentMode="scaleToFill" insetsLayoutMarginsFromSafeArea="NO" translatesAutoresizingMaskIntoConstraints="NO" id="tIm-aj-d8Q">
<rect key="frame" x="0.0" y="52" width="400" height="1348"/>
<rect key="frame" x="0.0" y="55" width="400" height="1346"/>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to move the ProjectPamphletContent container down a little bit because it was overlapping the top cell.

@ksr-ci-bot
Copy link

ksr-ci-bot commented Nov 6, 2019

1 Warning
⚠️ Big PR

SwiftFormat found issues:

File Rules
Kickstarter-iOS/Views/Cells/ProjectPamphletCreatorHeaderCell.swift anyObjectProtocol

Generated by 🚫 Danger

)
}

final class ProjectPamphletCreatorHeaderCell: UITableViewCell, ValueCell {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still not sure about this name πŸ€” Would ProjectPamphletViewProgressCell be better?

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 questions/comments but looks good!


private func attributedLaunchDateString(with project: Project)
-> NSAttributedString? {
let date = Format.date(secondsInUTC: project.dates.launchedAt, dateStyle: .long, timeStyle: .none)
Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to pass a timezone of UTC to this date formatter so that the date doesn't get converted to the device's timezone? πŸ€”

}

func testLaunchDateLabelAttributedText() {
let project = Project.template
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we set the .launchedAt variable on the project to a date we've defined so we can see that it's actually displaying the correct date? Otherwise this test feels a little hand-wavey.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats a good idea!

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant more something like:

   // Set the date to Nov 1, 2019
    let dateComponents = DateComponents()
      |> \.month .~ 11
      |> \.day .~ 1
      |> \.year .~ 2_019
      |> \.timeZone .~ TimeZone.init(secondsFromGMT: 0)

      let date = AppEnvironment.current.calendar.date(from: dateComponents)

      let project = Project.template |> \.dates.launchedAt .~ date.timeIntervalSince1970
...
    // Assert that the date is Nov 1, 2019, which is the date you set in the "launchedAt" property
    self.launchDateLabelAttributedText.assertValue("You launched this project on November 1, 2019.")

You might also want to set the locale in withEnvironment to ensure that the date is always formatted in the same way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohh I see!

@@ -4,6 +4,7 @@ import Prelude

internal final class ProjectPamphletContentDataSource: ValueCellDataSource {
internal enum Section: Int {
case viewProgress
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'd name this a little closer to the cell it represents, maybe creatorHeader?


private enum Layout {
enum Button {
static let height: CGFloat = 48
Copy link
Contributor

Choose a reason for hiding this comment

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

Where are these constants from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comes from Abstract.

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.

We might want to increase the stack view spacing, it looks a little tight on iPhone 5s:

image


self.viewModel.outputs.notifyDelegateViewProgressButtonTapped
.observeForUI()
.observeValues { [weak self] in
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we name the argument here? We typically name them when observing the values in bindViewModel to make it clear what the value is.

}

func testLaunchDateLabelAttributedText() {
let project = Project.template
Copy link
Contributor

Choose a reason for hiding this comment

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

I meant more something like:

   // Set the date to Nov 1, 2019
    let dateComponents = DateComponents()
      |> \.month .~ 11
      |> \.day .~ 1
      |> \.year .~ 2_019
      |> \.timeZone .~ TimeZone.init(secondsFromGMT: 0)

      let date = AppEnvironment.current.calendar.date(from: dateComponents)

      let project = Project.template |> \.dates.launchedAt .~ date.timeIntervalSince1970
...
    // Assert that the date is Nov 1, 2019, which is the date you set in the "launchedAt" property
    self.launchDateLabelAttributedText.assertValue("You launched this project on November 1, 2019.")

You might also want to set the locale in withEnvironment to ensure that the date is always formatted in the same way.

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.

lgtm!

@Scollaco Scollaco merged commit f4800ce into master Nov 7, 2019
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

4 participants