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

Fix #1649 Extending Onboarding UI Below Safe Area #1681

Closed

Conversation

@chickdan
Copy link
Contributor

chickdan commented Oct 15, 2019

Summary of Changes

This change sets the bottom of the modal equal to the superview and provides a new content inset for the description stack view to account for the size of the safe area.

Regarding the centering of the button noted in the issue: I noticed the button is off center on all of the onboarding modals and feel like the change(s) could potentially be large enough to be out of scope.

This pull request fixes issue #1649

Submitter Checklist:

  • Unit Tests are updated to cover new or changed functionality
  • User-facing strings use NSLocalizableString()

Test Plan:

Screenshots:

Simulator Screen Shot - iPhone Xs - 2019-10-14 at 23 53 28

Reviewer Checklist:

  • Issues include necessary QA labels:
    • QA/(Yes|No)
    • release-notes/(include|exclude)
    • bug / enhancement
  • Necessary security reviews have taken place.
  • Adequate unit test coverage exists to prevent regressions.
  • Adequate test plan exists for QA to validate (if applicable).
  • Issue is assigned to a milestone (should happen at merge time).
@iccub iccub requested a review from Brandon-T Oct 15, 2019
@@ -13,6 +13,7 @@ extension OnboardingAdsCountdownViewController {
/// A negative spacing is needed to make rounded corners for details view visible.
static let negativeSpacing: CGFloat = -16
static let descriptionContentInset: CGFloat = 32
static let descriptionContentBottomInset: CGFloat = 52

This comment has been minimized.

Copy link
@Brandon-T

Brandon-T Oct 15, 2019

Collaborator

We shouldn't hardcode the safe-area inset. See below comment :)

@@ -112,12 +113,13 @@ extension OnboardingAdsCountdownViewController {
mainStackView.snp.makeConstraints {

This comment has been minimized.

Copy link
@Brandon-T

Brandon-T Oct 15, 2019

Collaborator

This would change to:

mainStackView.snp.makeConstraints {
    $0.leading.trailing.bottom.equalToSuperview()
}
}

descriptionView.addSubview(descriptionStackView)
descriptionStackView.snp.makeConstraints {
$0.edges.equalToSuperview().inset(UX.descriptionContentInset)
$0.top.leading.trailing.equalToSuperview().inset(UX.descriptionContentInset)

This comment has been minimized.

Copy link
@Brandon-T

Brandon-T Oct 15, 2019

Collaborator

and this would change to:

descriptionStackView.snp.makeConstraints {
    $0.edges.equalTo(descriptionView.safeArea.edges).inset(UX.descriptionContentInset)
}

Now you don't have to worry about "52" :). It will automatically calculate how far from the bottom it should be.

@Brandon-T
Copy link
Collaborator

Brandon-T commented Oct 15, 2019

For this particular ticket, when we change it to extend the safe-area insets, we also have to update the animation in OnboardingNavigationController line: 244

@iccub
Copy link
Contributor

iccub commented Oct 15, 2019

This will get fixed in #1684, sorry for that.

Brandon assigned himself to this task few days ago, for contributors it's usually the best to take tasks without assignments or ask if it can be picked up in case someones is assigned to it.

Really appreciate your help, your first PR #1628 should get merged in after one more round of code review :)

@iccub iccub closed this Oct 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.