Quick Start: Fix orientation-related issues with quick start notices by hassaanelgarem · Pull Request #18758 · wordpress-mobile/WordPress-iOS

hassaanelgarem · 2022-05-27T02:04:46Z

Fixes #18493
Fixes #18757

Description

Fixes an issue where the notice was filing the whole screen width on large devices
Fixes an issue where the notice would get misaligned after changing orientation.
- The root cause was due to the UIDevice.orientationDidChangeNotification notification being posted before the tab bar has updated its height. And since we depend on the tab bar height to align the notice, this causes the misalignment.

Testing Instructions

Notice Width

Test on a large device (13 Pro Max)
Enable quick start for existing sites
Put the device in landscape mode
Start notifications task
Make sure that the notice fills 50% of the whole screen
Rotate the device to portrait
Make sure that the notice fills 100% of the whole screen
Rotate again
Make sure that the notice fills 50% of the whole screen

Notice Misalignment

Test on iPhone 13 Pro Max or XS
Enable quick start for existing sites
Start notifications task
Note the position of the notice
Keep changing the orientation
Make sure the distance between notice and tab bar is always the same

Regression Notes

Potential unintended areas of impact
N/A
What I did to test those areas of impact (or what existing automated tests I relied on)
N/A
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.

momo-ozawa

@hassaanelgarem 👋
I'm getting a build error:

❌  /usr/local/var/buildkite-agent/builds/builder/automattic/wordpress-ios/WordPress/Classes/ViewRelated/System/WPTabBarController.m:573:15: no visible @interface for 'WPTabBarController' declares the selector 'notifyOfTabBarHeightChangeIfNeeded'

momo-ozawa

If the initial screen is site menu and I rotate landscape -> portrait -> landscape, two unexpected things happen:

The notice isn't shown any more
The FAB is "stuck" in the adjusted position when notices are visible

Simulator.Screen.Recording.-.iPhone.13.Pro.Max.-.2022-05-27.at.11.48.56.mp4

WordPress/Classes/ViewRelated/System/WPTabBarController.m

peril-wordpress-mobile · 2022-05-29T14:13:58Z

	Warnings
⚠️	This PR contains changes to `RELEASE_NOTES.txt`. Note that these changes won't affect the final version of the release notes as this version is in code freeze. Please, get in touch with a release manager if you want to update the final release notes.

Generated by 🚫 dangerJS

wpmobilebot · 2022-05-29T14:37:45Z

You can test the changes in Jetpack from this Pull Request by:

Clicking here or scanning the QR code below to access App Center
Then installing the build number pr18758-028e2bc on your iPhone

If you need access to App Center, please ask a maintainer to add you.

wpmobilebot · 2022-05-29T14:40:15Z

You can test the changes in WordPress from this Pull Request by:

Clicking here or scanning the QR code below to access App Center
Then installing the build number pr18758-028e2bc on your iPhone

If you need access to App Center, please ask a maintainer to add you.

hassaanelgarem · 2022-05-29T22:46:23Z

@momo-ozawa

If the initial screen is site menu and I rotate landscape -> portrait -> landscape, two unexpected things happen:

That was caused by the stats appearing then disappearing. Fixed in bee31c3 👍

mokagio · 2022-05-30T00:02:07Z

@hassaanelgarem @momo-ozawa Notice I updated the base branch for this PR to release/20.0. The PR clearly looks like something valuable to have in 20.0, so I'd rather ship another beta just for it than have our users wait an additional four weeks to get this fix.

Feel free to revert to trunk if I misunderstood.

momo-ozawa

@hassaanelgarem I'm seeing two new issues 👀

If you don't find an immediate solution for this, what do you think of switching back to target trunk? AFAIK this issue has been present for a while now / it's not a regression we introduced recently.

Rotating to landscape when the initial screen is Home

The animation to switch to the split vc doesn't happen immediately -- you see a black screen where the detail vc should be. Then the primary pane rotates to the wrong angle. Finally, the screen switches to the intended split vc view.

Simulator.Screen.Recording.-.iPhone.13.Pro.Max.-.2022-05-31.at.09.43.19.mp4

Rotating to landscape when the initial screen is Menu

The segmented control is superimposed on top of the Next Steps section title before disappearing.

Simulator.Screen.Recording.-.iPhone.13.Pro.Max.-.2022-05-31.at.09.44.17.mp4

hassaanelgarem · 2022-05-31T15:28:30Z

@momo-ozawa

The animation to switch to the split vc doesn't happen immediately -- you see a black screen where the detail vc should be. Then the primary pane rotates to the wrong angle. Finally, the screen switches to the intended split vc view.

That's weird but I can't reproduce this at all. I've tested on 13 Pro Max iOS 15.4. What iOS version are you testing with?

The segmented control is superimposed on top of the Next Steps section title before disappearing.

Nice catch! I'll look into that, but I might end up opening another PR for this.

If you don't find an immediate solution for this, what do you think of switching back to target trunk? AFAIK this issue has been present for a while now / it's not a regression we introduced recently.

If these issues are already existent (The second one is existent on trunk), I'd prefer to ship the fixes we have now and address the new issues separately in another PR(s).

momo-ozawa · 2022-05-31T15:47:12Z

@hassaanelgarem

That's weird but I can't reproduce this at all. I've tested on 13 Pro Max iOS 15.4. What iOS version are you testing with?

I'm testing on a 13 Pro Max iOS 15.4 simulator. I'm using Xcode 13.3.0. Which Xcode version are you using?

If these issues are already existent (The second one is existent on trunk), I'd prefer to ship the fixes we have now and address the new issues separately in another PR(s).

Sure, sounds good to me. I just tested the 19.9 tag and I was able to reproduce this behavior, which indicates that neither of the issues are regressions. So should be safe to address these in different PRs.

One last thing -- let's make sure to add an [internal] note about this 👍

hassaanelgarem · 2022-05-31T16:32:39Z

@momo-ozawa

Which Xcode version are you using?

13.3.0 as well 🤔

So should be safe to address these in different PRs.

Awesome!

let's make sure to add an [internal] note about this

Sure! Done in 028e2bc 👍

momo-ozawa

LGTM!

Tested the steps again and can confirm that this issue is fixed!

mokagio · 2022-06-02T04:11:11Z

@hassaanelgarem Unless there's an urgent need to ship this in a beta, I'd wait until another PR lands on the release branch. I know there's one coming with a fix, but I don't think it has been opened yet.

Let me know if that's okay. If it isn't, I'll ship a beta with this ASAP.

cc @tiagomar

hassaanelgarem · 2022-06-02T12:21:41Z

@mokagio I agree! This isn't urgent indeed 👍

mokagio · 2022-06-03T06:39:00Z

@hassaanelgarem this has been bundled as part of 20.0 beta 1 (20.0.0.1).

Thanks for your work 🙌

hassaanelgarem added 2 commits May 27, 2022 03:48

Fix: correctly place notices when orientation changes

a3318c0

Fix: activate notice width constraint when notice is added if needed

d9a80e9

hassaanelgarem added [Type] Bug labels May 27, 2022

hassaanelgarem requested review from leandroalonso and momo-ozawa May 27, 2022 02:04

hassaanelgarem self-assigned this May 27, 2022

hassaanelgarem mentioned this pull request May 27, 2022

Quick Start: Rotating device can misalign notification of task completion #18493

Closed

momo-ozawa reviewed May 27, 2022

View reviewed changes

WordPress/Classes/ViewRelated/System/WPTabBarController.m Outdated Show resolved Hide resolved

Fix: typo in WPTabBarController

2fe0493

hassaanelgarem added this to the 20.0 milestone May 29, 2022

Fix: avoid dismissing notices when not needed

bee31c3

mokagio changed the base branch from trunk to rename-create_release-create_gh_release May 30, 2022 00:02

mokagio changed the base branch from rename-create_release-create_gh_release to release/20.0 May 30, 2022 00:02

hassaanelgarem requested a review from momo-ozawa May 31, 2022 02:06

momo-ozawa reviewed May 31, 2022

View reviewed changes

Add: release note

028e2bc

hassaanelgarem requested a review from momo-ozawa June 1, 2022 13:32

momo-ozawa approved these changes Jun 1, 2022

View reviewed changes

hassaanelgarem merged commit c61fe13 into release/20.0 Jun 2, 2022

hassaanelgarem deleted the issue/18493-qs-notices-landscape branch June 2, 2022 02:23

mokagio mentioned this pull request Jun 3, 2022

Merge 20.0.0.1 beta with conflicts resolution #18825

Merged

7 tasks

momo-ozawa mentioned this pull request Jan 31, 2023

Quick Start: Notices sometimes have full width on large devices #18757

Closed

Conversation

hassaanelgarem commented May 27, 2022

Description

Testing Instructions

Notice Width

Notice Misalignment

Regression Notes

Uh oh!

momo-ozawa left a comment

Choose a reason for hiding this comment

Uh oh!

momo-ozawa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

peril-wordpress-mobile bot commented May 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented May 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wpmobilebot commented May 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hassaanelgarem commented May 29, 2022

Uh oh!

mokagio commented May 30, 2022

Uh oh!

momo-ozawa left a comment

Choose a reason for hiding this comment

Rotating to landscape when the initial screen is Home

Rotating to landscape when the initial screen is Menu

Uh oh!

hassaanelgarem commented May 31, 2022

Uh oh!

momo-ozawa commented May 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hassaanelgarem commented May 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

momo-ozawa left a comment

Choose a reason for hiding this comment

Uh oh!

mokagio commented Jun 2, 2022

Uh oh!

hassaanelgarem commented Jun 2, 2022

Uh oh!

mokagio commented Jun 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

peril-wordpress-mobile bot commented May 29, 2022 •

edited

Loading

wpmobilebot commented May 29, 2022 •

edited

Loading

wpmobilebot commented May 29, 2022 •

edited

Loading

momo-ozawa commented May 31, 2022 •

edited

Loading

hassaanelgarem commented May 31, 2022 •

edited

Loading