-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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-206 - [1/4] Reward received toggle - UI #844
Conversation
import Prelude | ||
import UIKit | ||
|
||
final class ManageViewPledgeRewardReceivedViewController: ToggleViewController { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're using ToggleViewController
as a base class it could be reused in the future and this controller should be able to provide it's own logic (which API call will be called) + styling.
|
||
let (parent, _) = traitControllers(device: device, orientation: .portrait, child: controller) | ||
|
||
parent.view.frame.size.height = 60 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More real time scenario and should make the snapshot smaller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if would make sense for us to wait until the manage/view pledge screen is finished in order to create snapshot test for just that viewController instead of one for each child. This way we would avoid testing the same screen twice 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm not sure which side I lean towards more...I think each controller should be testable by itself but agree that as soon as we add manage pledge screen tests it will be a duplicate.
We already do this with SortPagerViewControllerTests
or ProjectNavBarViewControllerTests
which test just the menu itself and then we also have a test for the container ProjectPamphletContentViewControllerConversionTests
.
I believe testing each controller is more correct, since its parent can additionally style it or decide what it needs to do with it.
super.viewDidLoad() | ||
|
||
_ = self.titleLabel | ||
|> ksr_setContentCompressionResistancePriority(.defaultLow, for: .horizontal) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sure that the titleLabel
does not push the switch off screen and rather pushes its own text on next line.
combos([Language.en], devices).forEach { language, device in | ||
withEnvironment(language: language) { | ||
let controller = ToggleViewController.instantiate() | ||
controller.titleLabel.text = "Title for testing purposes only" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the base class doesn't provide default title or ON/OFF state we're setting it here simply for the purposes of our tests.
} | ||
} | ||
|
||
func testViewSupportsDynamicType() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've also decided to add Dynamic type snapshot in order to keep a regression test for the resizing of the label. This should only be done on the base class (no need for subclasses to handle this)
|
||
private func setupConstraints() { | ||
NSLayoutConstraint.activate([ | ||
self.rootStackView.widthAnchor.constraint(equalTo: self.rootScrollView.widthAnchor) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You usually need to also set the width of the scroll view to be the equal to the width of the parent VC 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I know we've done it before but it didn't seem necessary this time ... same on PledgeVC
🤔 🐒
|> \.alwaysBounceVertical .~ true | ||
} | ||
|
||
private let rootStackViewStyle = { (stackView: UIStackView) in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In PledgeViewController
we also have a rootStackViewStyle
: https://github.com/kickstarter/ios-oss/blob/master/Kickstarter-iOS/Views/Controllers/PledgeViewController.swift#L402
I wonder if it be worthwhile to extract this style into our CheckoutStyles.swift
file so they can be shared? I'd imagine that the pledge screen and the manage pledge screen should probably have consistent root stack view styles regardless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LOL, there's so many repeated styles..I'll try to consolidate these into something more reusable 😄
self.titleLabel.bottomAnchor.constraint(equalTo: self.view.bottomAnchor), | ||
self.titleLabel.centerYAnchor.constraint(equalTo: self.toggle.centerYAnchor), | ||
self.titleLabel.rightAnchor.constraint(lessThanOrEqualTo: self.toggle.leftAnchor), | ||
self.toggle.rightAnchor.constraint(equalTo: self.view.rightAnchor) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the toggle need a vertical position constraint? 🤔 Looks like there isn't one set right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
centerY
?
@@ -39,6 +39,15 @@ final class CheckoutStylesTests: XCTestCase { | |||
XCTAssertEqual(9, stackView.spacing) | |||
} | |||
|
|||
func testCheckoutSwitchControlStyle() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't think these tests are necessary 😬 Anytime we decide to change a small aspect of the checkout UI these tests break - it seems like it adds unnecessary maintenance for something that's already covered with other tests (snapshot tests) and offers only a marginal increase in test coverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. I just left a few question/comments 👍
@@ -68,6 +93,15 @@ final class ManageViewPledgeViewController: UIViewController { | |||
_ = self.closeButton | |||
|> \.accessibilityLabel %~ { _ in Strings.Dismiss() } | |||
|> \.width .~ Styles.minTouchSize.width | |||
|
|||
_ = self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The title should be handled by the view model, because it depends on the project state (live vs finished)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies, completely missed this
|
||
let (parent, _) = traitControllers(device: device, orientation: .portrait, child: controller) | ||
|
||
parent.view.frame.size.height = 60 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if would make sense for us to wait until the manage/view pledge screen is finished in order to create snapshot test for just that viewController instead of one for each child. This way we would avoid testing the same screen twice 🤔
Library/Styles/CheckoutStyles.swift
Outdated
public let checkoutSwitchControlStyle: SwitchControlStyle = { switchControl in | ||
switchControl | ||
|> \.onTintColor .~ UIColor.ksr_green_500 | ||
|> \.tintColor .~ UIColor.ksr_green_500 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the tintColor
supposed to be green? Our app usually adopts grey for the off
state. Maybe a ksr_grey_500
or darker would fit better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the design was kinda weird because it wasn't using the native switch component...let me run this by Nneka
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to tidy up the styles into more reusable styles, I've discovered that some of the child controller (sub-controllers on the pledge screen) use different spacing for their sub-components (like stepper, buttons, labels, etc.)
I wonder if we should simplify this and make it consistent...This would mean the vertical spacing (in points) would be the same for these sections (highlighted bellow in red).
Before and after would look something like this:
Before | After |
---|---|
This is something we would have probably spotted if we had a designer. It's always a good practice to keep things consistent between components that construct the same page because it's easier to thing about these in the future and makes changes easier to do.
@@ -112,7 +112,6 @@ final class PledgeCreditCardView: UIView { | |||
|
|||
private let adaptableStackViewStyle: StackViewStyle = { stackView in | |||
stackView | |||
|> \.backgroundColor .~ UIColor.white |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the docs...
UIStackView is a non-drawing view, meaning that drawRect() is never called and its background color is ignored. If you desperately want a background color, consider placing the stack view inside another UIView and giving that view a background color.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically all these backgroundColor
styles were obsolete so I've removed them
Generated by 🚫 Danger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree that we should be consistent. This would help us to define baseStyles and make the styling process much simpler without the need to tweak the spacing for each subview. |
📲 What
Basic UI for the reward received toggle.
🤔 Why
Step one in showing the toggle is to actually have something to show 😉 so this is the UI itself.
🛠 How
It's been pretty challening making this work with a stack view - apparently multi-line labels and stack views are hard - who'd knew 🤦♂.
Because of that we're simply using auto layout and we're not adjusting axis for accessibility traits. Even though this causes the lack of axis adjustment often seen in the Settings.app (see bellow) where the toggle would align on the next line it's a small trade off to make. The toggle's width makes it acceptable staying at it's position at all times while adjusting just the label. We can potentially address this in the future with more customized control.
👀 See
.large
.extraExtraExtraLarge
.accessibilityExtraExtraExtraLarge
.large
.extraExtraExtraLarge
.accessibilityExtraExtraExtraLarge
.large
.extraExtraExtraLarge
.accessibilityExtraExtraExtraLarge
♿️ Accessibility
VoiceOver
accessibility will be addressed in a follow up PR since it showed up quite challenging.✅ Acceptance criteria
Profile > Backed project > Manage
⏰ TODO
(will be addressed in follow up PRs)