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-227] Cancel Pledge UI #864

Merged
merged 29 commits into from
Oct 3, 2019
Merged

[NT-227] Cancel Pledge UI #864

merged 29 commits into from
Oct 3, 2019

Conversation

ifbarrera
Copy link
Contributor

@ifbarrera ifbarrera commented Oct 1, 2019

📲 What

Builds the UI components of the cancel pledge screen. Note that this PR does not include the text field functionality, that will be added as part of the implementation for the full cancel pledge UX.

🤔 Why

For the Cancel Pledge feature.

🛠 How

Adds all UI elements as part of a root stack view. Notice that here I've preferred to use setCustomSpacing(after) to tweak spacing between certain views, instead of creating sub-stackviews. Open to feedback on that approach.

Note that the following will be addressed in a follow-up PR:

  • accessibility label on the cancellation reason text field
  • highlight/disabled states of the redButtonStyle are TBD

👀 See

Trello, screenshots, external resources?

iPhone 8 Dynamic Type Landscape
Yw0lzWq6qo Simulator Screen Shot - iPhone 8 - 2019-10-01 at 13 03 42 am9FEHVUT5

♿️ Accessibility

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

✅ Acceptance criteria

  • Navigate to a backed project with the "Native Checkout" and "Native Checkout Pledge View" feature flags turned on. Tap the menu button on the top right. Select "Cancel Pledge". You should be taken to the cancel pledge screen. Tap "No, go back". You should be taken back to the Manage Pledge screen.

Copy link
Contributor

@dusi dusi left a comment

Choose a reason for hiding this comment

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

Couple minor suggestions / questions and than was wondering about UX when a user might have font set to a larger size (maybe on a smaller device) which would result into keyboard overlapping the input field...so the question is - should we remove the top space when the keyboard is on as well (not only when the traits are accessibility+)?

See bellow

Screen Shot 2019-10-02 at 3 18 25 PM

Comment on lines +220 to +221
|> \.textAlignment .~ NSTextAlignment.center
|> \.numberOfLines .~ 0
Copy link
Contributor

Choose a reason for hiding this comment

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

These could technically be reused for multi-line centered style ¯_(ツ)_/¯

@@ -34,6 +34,14 @@ public let apricotButtonStyle = baseButtonStyle
<> UIButton.lens.backgroundColor(for: .highlighted) .~ UIColor.ksr_apricot_500.mixDarker(0.12)
<> UIButton.lens.backgroundColor(for: .disabled) .~ UIColor.ksr_apricot_500.mixLighter(0.36)

// MARK: - Red

public let redButtonStyle = baseButtonStyle
Copy link
Contributor

Choose a reason for hiding this comment

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

My apologies, I removed this previously since we didn't use it

@ifbarrera
Copy link
Contributor Author

Couple minor suggestions / questions and than was wondering about UX when a user might have font set to a larger size (maybe on a smaller device) which would result into keyboard overlapping the input field...so the question is - should we remove the top space when the keyboard is on as well (not only when the traits are accessibility+)?

See bellow

Screen Shot 2019-10-02 at 3 18 25 PM

I'd hate to have a bunch of custom UI configuration for dynamic sizing + smaller devices etc. Although it overlaps the text field on smaller devices with larger sizes set, the contentInset.bottom is still updated to accommodate the presence of the keyboard, which means all a user would have to do is scroll the text field into place. This feels acceptable enough for such a small edge case. What do you think?

@ifbarrera ifbarrera requested a review from dusi October 3, 2019 16:55
@ksr-ci-bot
Copy link

ksr-ci-bot commented Oct 3, 2019

1 Warning
⚠️ Big PR

Generated by 🚫 Danger

@dusi
Copy link
Contributor

dusi commented Oct 3, 2019

I'd hate to have a bunch of custom UI configuration for dynamic sizing + smaller devices etc. Although it overlaps the text field on smaller devices with larger sizes set, the contentInset.bottom is still updated to accommodate the presence of the keyboard, which means all a user would have to do is scroll the text field into place. This feels acceptable enough for such a small edge case. What do you think?

I see your point Isabel and I don't have strong feelings so I'd be OK with this approach...one other edge case will be landscape mode where basically the textfield will be completely hidden...I mentioned this because the implementation is in my opinion trivial to achieve.

Copy link
Contributor

@dusi dusi left a comment

Choose a reason for hiding this comment

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

🚢 🚢 🚢
🚢 🚢 🚢
🚢 🚢 🚢

@ifbarrera ifbarrera merged commit 3d7825d into master Oct 3, 2019
@ifbarrera ifbarrera deleted the cancel-backing-UI branch October 3, 2019 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants