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.

Sign up for GitHub

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

Jump to bottom

💲[Native Checkout] Project Pledge States UI - UI Fixes #724

Merged
merged 5 commits into from
Jun 27, 2019
Merged

💲[Native Checkout] Project Pledge States UI - UI Fixes #724

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
678e901
Add signal that would emit whether or not the spacer is hidden
dusi Jun 26, 2019
79abf80
Trim the amount
dusi Jun 26, 2019
6fd29d2
Fix UI issues with Dynamic Type
dusi Jun 26, 2019
f89e735
Revert back to the right font
dusi Jun 26, 2019
5ccd27a
Rename button and style
dusi Jun 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 68 additions & 38 deletions Kickstarter-iOS/Views/PledgeCTAContainerView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,27 @@ final class PledgeCTAContainerView: UIView {

private let vm: PledgeCTAContainerViewViewModelType = PledgeCTAContainerViewViewModel()

private lazy var amountAndRewardTitleStackView: UIStackView = { UIStackView(frame: .zero) }()
private lazy var amountAndRewardTitleStackView: UIStackView = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Enabling auto layout in initializers thorough this file so that we don't have to combine it with style

UIStackView(frame: .zero)
|> \.translatesAutoresizingMaskIntoConstraints .~ false
}()

private lazy var amountOrRewardLabel: UILabel = { UILabel(frame: .zero) }()
private lazy var pledgeCTAbutton: UIButton = {
private lazy var pledgeCTAButton: UIButton = {
MultiLineButton(type: .custom)
|> \.translatesAutoresizingMaskIntoConstraints .~ false
}()

private lazy var rootStackView: UIStackView = { UIStackView(frame: .zero) }()
private lazy var rootStackView: UIStackView = {
UIStackView(frame: .zero)
|> \.translatesAutoresizingMaskIntoConstraints .~ false
}()

private lazy var spacer: UIView = {
UIView(frame: .zero)
|> \.translatesAutoresizingMaskIntoConstraints .~ false
}()

private lazy var youreABackerLabel: UILabel = { UILabel(frame: .zero) }()

// MARK: - Lifecycle
Expand All @@ -30,17 +43,12 @@ final class PledgeCTAContainerView: UIView {
_ = ([self.youreABackerLabel, self.amountOrRewardLabel], self.amountAndRewardTitleStackView)
|> ksr_addArrangedSubviewsToStackView()

_ = ([self.amountAndRewardTitleStackView, self.pledgeCTAbutton], self.rootStackView)
_ = ([self.amountAndRewardTitleStackView, self.spacer, self.pledgeCTAButton], self.rootStackView)
|> ksr_addArrangedSubviewsToStackView()

NSLayoutConstraint.activate(
[
self.pledgeCTAbutton.heightAnchor.constraint(
greaterThanOrEqualToConstant: Styles.minTouchSize.height
),
self.pledgeCTAbutton.trailingAnchor.constraint(equalTo: self.rootStackView.trailingAnchor)
Copy link
Contributor Author

@dusi dusi Jun 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Before 🐛 After 🦋
Screen Shot 2019-06-25 at 5 50 46 PM Screen Shot 2019-06-25 at 5 49 33 PM

This constraint caused the button to align to the edge of the stack view. This was easily reproducible if you made dynamic type font larger and opened any pledge detail screen.

Copy link
Contributor

Choose a reason for hiding this comment

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

I did not reproduce this but it could have been pointed out in #702.

]
)
NSLayoutConstraint.activate([
self.pledgeCTAButton.heightAnchor.constraint(greaterThanOrEqualToConstant: Styles.minTouchSize.height)
])

self.bindViewModel()
}
Expand All @@ -54,41 +62,44 @@ final class PledgeCTAContainerView: UIView {
override func bindStyles() {
super.bindStyles()

_ = self.pledgeCTAbutton
|> projectStateButtonStyle
let isAccessibilityCategory = self.traitCollection.preferredContentSizeCategory.isAccessibilityCategory

_ = self.youreABackerLabel
|> \.font .~ .ksr_headline(size: 14)
|> \.text %~ { _ in Strings.Youre_a_backer() }
_ = self.amountAndRewardTitleStackView
|> \.axis .~ NSLayoutConstraint.Axis.vertical
|> \.isLayoutMarginsRelativeArrangement .~ true

_ = self.amountOrRewardLabel
|> \.font .~ .ksr_caption1(size: 14)
|> \.textColor .~ .ksr_dark_grey_500
|> \.font .~ UIFont.ksr_caption1(size: 14)
|> \.textColor .~ UIColor.ksr_dark_grey_500
|> \.numberOfLines .~ 0
Copy link
Contributor Author

@dusi dusi Jun 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Before 🐛 After 🦋
Screen Shot 2019-06-25 at 5 49 33 PM copy Screen Shot 2019-06-25 at 5 50 21 PM copy

Allows the label to fit multiple lines if it has long text

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we should to handle Dynamic Type on fixed elements differently. The viewable part of the screen (beneath the PledgeCTAContainerView) is getting pretty tight!


_ = self.pledgeCTAButton
|> pledgeCTAButtonStyle(
isAccessibilityCategory,
amountAndRewardTitleStackViewIsHidden: self.amountAndRewardTitleStackView.isHidden
)

_ = self.rootStackView
|> checkoutAdaptableStackViewStyle(
self.traitCollection.preferredContentSizeCategory.isAccessibilityCategory
)
|> \.distribution .~ UIStackView.Distribution.equalCentering
|> \.translatesAutoresizingMaskIntoConstraints .~ false
|> adaptableStackViewStyle(isAccessibilityCategory)
|> \.isLayoutMarginsRelativeArrangement .~ true
|> \.layoutMargins .~ UIEdgeInsets.init(topBottom: Styles.grid(3), leftRight: Styles.grid(3))

_ = self.amountAndRewardTitleStackView
|> \.axis .~ NSLayoutConstraint.Axis.vertical
|> \.translatesAutoresizingMaskIntoConstraints .~ false
|> \.isLayoutMarginsRelativeArrangement .~ true
_ = self.youreABackerLabel
|> \.font .~ UIFont.ksr_headline(size: 14)
|> \.text %~ { _ in Strings.Youre_a_backer() }
|> \.numberOfLines .~ 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above

}

// MARK: - Binding

override func bindViewModel() {
super.bindViewModel()

self.pledgeCTAbutton.rac.title = self.vm.outputs.buttonTitleText
self.pledgeCTAbutton.rac.backgroundColor = self.vm.outputs.buttonBackgroundColor
self.amountAndRewardTitleStackView.rac.hidden = self.vm.outputs.stackViewIsHidden
self.amountOrRewardLabel.rac.text = self.vm.outputs.rewardTitle
self.pledgeCTAButton.rac.backgroundColor = self.vm.outputs.buttonBackgroundColor
self.pledgeCTAButton.rac.title = self.vm.outputs.buttonTitleText
self.spacer.rac.hidden = self.vm.outputs.spacerIsHidden
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We now also need to hide the spacer so we need 1 additional signal that should be tested as well

}

// MARK: - Configuration
Expand All @@ -100,12 +111,31 @@ final class PledgeCTAContainerView: UIView {

// MARK: - Styles

private let projectStateButtonStyle: ButtonStyle = { (button: UIButton) in
button
|> roundedStyle(cornerRadius: 12)
|> UIButton.lens.titleColor(for: .normal) .~ .white
|> UIButton.lens.titleLabel.font .~ .ksr_headline(size: 15)
|> UIButton.lens.layer.borderWidth .~ 0
|> UIButton.lens.titleEdgeInsets .~ .init(topBottom: Styles.grid(1), leftRight: Styles.grid(2))
|> (UIButton.lens.titleLabel .. UILabel.lens.lineBreakMode) .~ .byWordWrapping
private func adaptableStackViewStyle(_ isAccessibilityCategory: Bool) -> (StackViewStyle) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using private style because we were only using 50% of the global checkout style which we would use and then had to change 2 props back to default...so this should be simpler

return { (stackView: UIStackView) in
let axis: NSLayoutConstraint.Axis = (isAccessibilityCategory ? .vertical : .horizontal)
let spacing: CGFloat = (isAccessibilityCategory ? Styles.grid(1) : 0)

return stackView
|> \.axis .~ axis
|> \.spacing .~ spacing
}
}

private func pledgeCTAButtonStyle(
_ isAccessibilityCategory: Bool, amountAndRewardTitleStackViewIsHidden: Bool
) -> (ButtonStyle) {
return { (button: UIButton) in
let lineBreakMode: NSLineBreakMode = isAccessibilityCategory || amountAndRewardTitleStackViewIsHidden
Copy link
Contributor Author

@dusi dusi Jun 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Before 🐛 After 🦋
Screen Shot 2019-06-25 at 5 49 02 PM Screen Shot 2019-06-25 at 5 50 09 PM

This hopefully fixes the issue of not using wordWrap when the button can fit properly. Maybe there's a way around with compression resistance or content hugging priorities but I didn't have much luck..Cc: @justinswart 🤔

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 we want to avoid compression and hugging priorities if possible, seems like a good enough solution to change the lineBreakMode

? NSLineBreakMode.byWordWrapping : NSLineBreakMode.byTruncatingTail

return button
|> roundedStyle(cornerRadius: 12)
|> UIButton.lens.titleColor(for: .normal) .~ UIColor.white
|> UIButton.lens.titleLabel.font .~ UIFont.ksr_headline(size: 15)
|> UIButton.lens.layer.borderWidth .~ 0
|> UIButton.lens.titleEdgeInsets .~ .init(topBottom: Styles.grid(1), leftRight: Styles.grid(2))
|> (UIButton.lens.titleLabel .. UILabel.lens.textAlignment) .~ NSTextAlignment.center
Copy link
Contributor Author

@dusi dusi Jun 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Before 🐛 After 🦋
Screen Shot 2019-06-25 at 5 48 36 PM Screen Shot 2019-06-25 at 5 49 56 PM

Fixes the centering

|> (UIButton.lens.titleLabel .. UILabel.lens.lineBreakMode) .~ lineBreakMode
}
}
2 changes: 1 addition & 1 deletion Library/PledgeStateCTAType.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ public enum PledgeStateCTAType {
}
}

public var stackViewIsHidden: Bool {
public var stackViewAndSpacerAreHidden: Bool {
switch self {
case .pledge, .viewBacking, .viewRewards:
return true
Expand Down
8 changes: 6 additions & 2 deletions Library/ViewModels/PledgeCTAContainerViewViewModel.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ public protocol PledgeCTAContainerViewViewModelOutputs {
var buttonBackgroundColor: Signal<UIColor, Never> { get }
var buttonTitleText: Signal<String, Never> { get }
var rewardTitle: Signal<String, Never> { get }
var spacerIsHidden: Signal<Bool, Never> { get }
var stackViewIsHidden: Signal<Bool, Never> { get }
}

Expand All @@ -35,10 +36,12 @@ public final class PledgeCTAContainerViewViewModel: PledgeCTAContainerViewViewMo
}
let backing = backingEvent.values()
let projectAndBacking = Signal.combineLatest(project, backing)
let stackViewAndSpacerAreHidden = pledgeState.map { $0.stackViewAndSpacerAreHidden }

self.buttonTitleText = pledgeState.map { $0.buttonTitle }
self.buttonBackgroundColor = pledgeState.map { $0.buttonBackgroundColor }
self.stackViewIsHidden = pledgeState.map { $0.stackViewIsHidden }
self.spacerIsHidden = stackViewAndSpacerAreHidden
self.stackViewIsHidden = stackViewAndSpacerAreHidden

self.rewardTitle = projectAndBacking
.map { (project, backing) -> String in
Expand All @@ -47,7 +50,7 @@ public final class PledgeCTAContainerViewViewModel: PledgeCTAContainerViewViewMo
basicPledge,
country: project.country,
omitCurrencyCode: project.stats.omitUSCurrencyCode
)
).trimmed()
Copy link
Contributor Author

@dusi dusi Jun 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Before 🐛 After 🦋
Screen Shot 2019-06-25 at 5 49 02 PM Screen Shot 2019-06-25 at 5 50 09 PM
Screen Shot 2019-06-25 at 5 49 33 PM Screen Shot 2019-06-25 at 5 50 21 PM

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 should probably be tested in our VM to make sure that the output is indeed trimmed. Current tests need better template values since those provided values are always trimmed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can pair on this

guard let rewardTitle = backing.reward?.title else { return "\(amount)" }
return "\(amount) • \(rewardTitle)"
}
Expand All @@ -64,6 +67,7 @@ public final class PledgeCTAContainerViewViewModel: PledgeCTAContainerViewViewMo
public let buttonBackgroundColor: Signal<UIColor, Never>
public let buttonTitleText: Signal<String, Never>
public let rewardTitle: Signal<String, Never>
public let spacerIsHidden: Signal<Bool, Never>
public let stackViewIsHidden: Signal<Bool, Never>
}

Expand Down
6 changes: 6 additions & 0 deletions Library/ViewModels/PledgeCTAContainerViewViewModelTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,15 @@ internal final class PledgeCTAContainerViewViewModelTests: TestCase {
let buttonBackgroundColor = TestObserver<UIColor, Never>()
let buttonTitleText = TestObserver<String, Never>()
let rewardTitle = TestObserver<String, Never>()
let spacerIsHidden = TestObserver<Bool, Never>()
let stackViewIsHidden = TestObserver<Bool, Never>()

internal override func setUp() {
super.setUp()
self.vm.outputs.buttonBackgroundColor.observe(self.buttonBackgroundColor.observer)
self.vm.outputs.buttonTitleText.observe(self.buttonTitleText.observer)
self.vm.outputs.rewardTitle.observe(self.rewardTitle.observer)
self.vm.outputs.spacerIsHidden.observe(self.spacerIsHidden.observer)
self.vm.outputs.stackViewIsHidden.observe(self.stackViewIsHidden.observer)
}

Expand All @@ -41,6 +43,7 @@ internal final class PledgeCTAContainerViewViewModelTests: TestCase {
self.buttonBackgroundColor.assertValues([manageCTAColor])
self.buttonTitleText.assertValues(["Manage"])
self.rewardTitle.assertValues(["$8 • Magic Lamp"])
self.spacerIsHidden.assertValues([false])
self.stackViewIsHidden.assertValues([false])
}
}
Expand All @@ -56,6 +59,7 @@ internal final class PledgeCTAContainerViewViewModelTests: TestCase {
self.vm.inputs.configureWith(project: project, user: user)
self.buttonBackgroundColor.assertValues([viewPledgeCTAColor])
self.buttonTitleText.assertValues([Strings.View_your_pledge()])
self.spacerIsHidden.assertValues([true])
self.stackViewIsHidden.assertValues([true])
}

Expand All @@ -68,6 +72,7 @@ internal final class PledgeCTAContainerViewViewModelTests: TestCase {
self.vm.inputs.configureWith(project: project, user: user)
self.buttonBackgroundColor.assertValues([pledgeCTAColor])
self.buttonTitleText.assertValues([Strings.Back_this_project()])
self.spacerIsHidden.assertValues([true])
self.stackViewIsHidden.assertValues([true])
}

Expand All @@ -81,6 +86,7 @@ internal final class PledgeCTAContainerViewViewModelTests: TestCase {
self.vm.inputs.configureWith(project: project, user: user)
self.buttonBackgroundColor.assertValues([viewRewardsCTAColor])
self.buttonTitleText.assertValues([Strings.View_rewards()])
self.spacerIsHidden.assertValues([true])
self.stackViewIsHidden.assertValues([true])
}
}