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 push prompt button overlapping on iPad #19304

Conversation

ipalm0423
Copy link
Contributor

@ipalm0423 ipalm0423 commented Sep 13, 2022

Fixes #19243

Description:

This PR fix the issue where the bottom button was overlapping with other labels on the view.

Root Cause:

The constraints setup is wrong on BloggingRemindersPushPromptViewController.configureConstraints.
The turnOnNotificationsButton should below the labels stackView, so the constants should use a negative value not positive -Metrics.edgeMargins.bottom

Fix:

  • Consider to adaptive to both iPhone/iPad cases, the top constraint should be greater than a margin.
  • The button height constraint should be constant.
Before After
iPad Q3  Q3-fix
iPhone Q3-phone Q3-fix-phone

Test Step:

  1. Open device settings in iPad
  2. Scroll the settings list and open WordPress.
  3. Tap Notifications.
  4. Turn off "Allow Notifications" if it's on.
  5. Launch the app.
  6. Navigate My Site → Site Settings. Tap Blogging Reminders.
  7. Try to set a reminder. The notification permission popup will appear.
  8. Notice the button should not overlap with the text.

Regression Notes

  1. Potential unintended areas of impact
  • Only impact the prompt view itself in the notification setting flow.
  1. What I did to test those areas of impact (or what existing automated tests I relied on)
  • Manually tested the changes on iPad and iPhone (screenshot above)
  1. What automated tests I added (or what prevented me from doing so)
    n/a

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Sorry, something went wrong.

@ipalm0423 ipalm0423 marked this pull request as draft September 13, 2022 18:40
@twstokes twstokes self-requested a review September 14, 2022 15:45
@ipalm0423 ipalm0423 force-pushed the fix/19243-fix-push-prompt-label-overlapping-for-iPad branch from c7bf9c7 to 420fca0 Compare September 20, 2022 16:19
@ipalm0423 ipalm0423 marked this pull request as ready for review September 20, 2022 16:28
Copy link
Contributor

@twstokes twstokes left a comment

Choose a reason for hiding this comment

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

👋 @ipalm0423 - would you mind catching this branch up with the latest on trunk? Typically this wouldn't be required without a merge conflict, but we just merged Xcode 14 support and it won't build in that environment until we have those changes.

Thanks!

@ipalm0423 ipalm0423 force-pushed the fix/19243-fix-push-prompt-label-overlapping-for-iPad branch from 420fca0 to 2a4789e Compare September 21, 2022 03:58
@ipalm0423
Copy link
Contributor Author

ipalm0423 commented Sep 21, 2022

👋 @ipalm0423 - would you mind catching this branch up with the latest on trunk? Typically this wouldn't be required without a merge conflict, but we just merged Xcode 14 support and it won't build in that environment until we have those changes.

Thanks!

@twstokes 👋
No problem, rebased to trunk. Thanks for the information.

@twstokes twstokes self-requested a review September 21, 2022 15:31
Copy link
Contributor

@twstokes twstokes left a comment

Choose a reason for hiding this comment

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

This LGTM @ipalm0423 - I just had one question in the review.

Thanks for including a clear description, screenshots, and testing steps. It makes sense that this fix wouldn't easily be unit-testable as it's changes to the UI.

Should step 8 of the testing steps be updated to reflect the expectations?

@twstokes
Copy link
Contributor

I've pushed up a branch to run CI since it's a requirement before we can merge. #19332

@ipalm0423
Copy link
Contributor Author

This LGTM @ipalm0423 - I just had one question in the review.

Thank you for reviewing!

Thanks for including a clear description, screenshots, and testing steps. It makes sense that this fix wouldn't easily be unit-testable as it's changes to the UI.

Agree, It is not easy for unit test. The view is independent, so manual testing could fully cover the change.

Should step 8 of the testing steps be updated to reflect the expectations?

Modified. Thanks for reminding me😄.

@ipalm0423 ipalm0423 force-pushed the fix/19243-fix-push-prompt-label-overlapping-for-iPad branch from 2a4789e to 105308f Compare September 22, 2022 08:31
Copy link
Contributor

@twstokes twstokes left a comment

Choose a reason for hiding this comment

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

LGTM @ipalm0423! I've pushed up the latest to the CI branch to run tests. Once those are successful we'll merge. 👍

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.

Layout problem on notification permission popup
2 participants