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-730] Pledge screen header #1033

Merged
merged 23 commits into from
Jan 17, 2020
Merged

[NT-730] Pledge screen header #1033

merged 23 commits into from
Jan 17, 2020

Conversation

Scollaco
Copy link
Contributor

📲 What

  • Replaces the Reward description session on the PledgeViewController.

🤔 Why

🛠 How

  • Disabling the custom animation from the rewards carousel to the pledge screen
  • Removing any code related to the old header
  • Added logic to show/hide estimated delivery date stackView if reward doesn't have deliverable items
  • Added new NSAttributedString extension to make it easier the process to add links (not HTML) to strings
  • Added tests and snapshots

👀 See

No Reward Reward w/o deliverables Reward w/ deliverables Dynamic type
Simulator Screen Shot - iPhone 8 - 2020-01-13 at 12 29 44 Simulator Screen Shot - iPhone 8 - 2020-01-13 at 12 30 08 Simulator Screen Shot - iPhone 8 - 2020-01-13 at 12 29 54 Simulator Screen Shot - iPhone 8 - 2020-01-13 at 12 58 27

♿️ Accessibility

  • Works with VoiceOver
  • Supports Dynamic Type

🏎 Performance

  • Optimized Blended Layers (screenshots)

✅ Acceptance criteria

Go to a live project and choose:

  • A NoReward reward. The header title should say "Back it because you believe in it, delivery info is hidden`.
  • A reward without deliverable items. The header should show the rewards title and the delivery info is hidden.
  • A reward with deliverable items. The header should show the estimated delivery info above the rewards title.

func snapshotData(withContainerView view: UIView) -> RewardPledgeTransitionSnapshotData? {
return self.descriptionViewController.snapshotData(withContainerView: view)
}
func beginTransition(_: UINavigationController.Operation) {}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of deleting all of this code, I decided to keep this structure here since we discussed in standup that we may revisit the custom animation in an Investment Day.

Copy link
Contributor

Choose a reason for hiding this comment

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

On second thought I think we should remove the animation, I think it's unlikely we'll make it work with this new layout.

@nativeksr
Copy link
Collaborator

1 Warning
⚠️ Big PR

Generated by 🚫 Danger

Copy link
Contributor

@justinswart justinswart left a comment

Choose a reason for hiding this comment

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

Looks good! Had one or two questions.


return .init(width: newWidth, height: newHeight)
}
guard let attributedString = checkoutAttributedLink(with: fullString) else { return nil }
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we still want to call checkoutAttributedLink here because that function is expecting an HTML string.

Where do we set the attributes for the link text, colour etc?

func snapshotData(withContainerView view: UIView) -> RewardPledgeTransitionSnapshotData? {
return self.descriptionViewController.snapshotData(withContainerView: view)
}
func beginTransition(_: UINavigationController.Operation) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

On second thought I think we should remove the animation, I think it's unlikely we'll make it work with this new layout.

.skipNil()
.map { project, reward in (project, .init(left: reward)) }
.map { _, reward in reward.title ?? Strings.Back_it_because_you_believe_in_it() }
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to check if title is nil and also whether its noReward? Just to be sure 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The NoReward only contains some of the properties of the Reward object, and it doesn't have the title. So the title, in this case, will always be nil.

Copy link
Contributor

@justinswart justinswart left a comment

Choose a reason for hiding this comment

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

Cool cool, removing the use of checkoutAttributedLink seems to have changed the font style, is this the correct style now? Perhaps we should confirm with @colleenmacd.

There is another file that is no longer need now since we removed the animation called UISpringTimingParameters+Convenience.swift.

@Scollaco
Copy link
Contributor Author

Cool cool, removing the use of checkoutAttributedLink seems to have changed the font style, is this the correct style now? Perhaps we should confirm with @colleenmacd.

There is another file that is no longer need now since we removed the animation called UISpringTimingParameters+Convenience.swift.

Thanks, @justinswart ! I made a small change on the NSAttributedString and now the style seems correct again.

Copy link

@colleenmacd colleenmacd left a comment

Choose a reason for hiding this comment

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

Looks good! In the reward container, is the padding between the edge of container and the text 18 dp? Looks a little large but might be my eyes @Scollaco

@Scollaco
Copy link
Contributor Author

Looks good! In the reward container, is the padding between the edge of container and the text 18 dp? Looks a little large but might be my eyes @Scollaco

It is... I double checked it, just in case.

Copy link
Contributor

@justinswart justinswart left a comment

Choose a reason for hiding this comment

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

Good job!

@Scollaco Scollaco merged commit aa73551 into master Jan 17, 2020
@Scollaco Scollaco deleted the NT-730_pledge-screen-header branch January 17, 2020 21:54
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