Skip to content

Conversation

@koke
Copy link
Member

@koke koke commented Dec 2, 2022

Closes: #8048

Description

In #8228 we started hiding the message without animation as the stats view scrolled. This PR both adds animation and simplifies the logic.

After wrestling with SwiftUI and Auto Layout for almost a week, this seems to be the solution that works for now:

  • We show/hide the whole header stack view as the navigation bar collapses instead of messing with individual views
  • Using affine transforms caused problems with UIHostingController. The header was rendered in the right place, but it was reserving the original space, not letting the stats grow upwards.
  • Updating the top constraint for the header view to a negative offset almost worked, but the header stack height would change as it moved, making it hard to guess how much we should offset it
  • The final solution was to detach the constraint pinning the bottom of the header view and the top of the content view, so the content can grow to fit the screen. Instead of moving the header, it stays behind as the content grows. We also animate making it transparent for a smoother transition
  • We use UIViewPropertyAnimator, so we can correctly cancel any animation in progress, in case the user starts scrolling in the other direction while the animation is in progress. That would stop the animation in the current position, and start a new one in the opposite direction.

Testing instructions

See pdfdoF-1uc-p2#comment-2581 for details of setting up your store to be eligible for the test JITM

With a US store that is eligible for the test JITM, on a debug/alpha build of the app

  1. Go to My store, and verify that the In-person card payments message is shown, and the store name shows below the "My store" title
  2. As you scroll down the view and the "My store" title becomes smaller, the store name and message should disappear with animation
  3. As you scroll back up and the "My store" title becomes large, the store name and message should reappear with animation
  4. Rotate to landscape, both the store name and message should not be visible regardless of scroll position on iPhone. On an iPad, it should behave similarly to iPhone portrait.

This also seems to fix the rotation issues described in #8228

Screenshots

Screen.Recording.2022-12-02.at.11.25.04.mov
Screen.Recording.2022-12-02.at.11.31.04.mov

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@koke koke changed the title Try/jitm animate take 2 [Just In Time Messages] Animate hiding of message in My Store screen Dec 2, 2022
@koke koke added type: enhancement A request for an enhancement. feature: jitm Related to Just In Time Messages labels Dec 2, 2022
@koke koke added this to the 11.5 milestone Dec 2, 2022
@koke koke requested a review from toupper December 2, 2022 10:41
@koke koke marked this pull request as ready for review December 2, 2022 10:41
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Dec 2, 2022

You can test the changes from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr8293-1f169d7 on your iPhone

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

@toupper toupper self-assigned this Dec 2, 2022
@toupper
Copy link
Contributor

toupper commented Dec 2, 2022

The code looks great and it tests (almost) well. I found a crash, I couldn't get the exact steps but it happened often to me:

  1. Start the app.
  2. Change orientation several times.
  3. Change the tab, and go back to My Store.
  4. Change orientation several times, then eventually it crashes with:
2022-12-02 13:08:43.324863+0100 WooCommerce[9685:1694886] *** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'An animator <UIViewPropertyAnimator(0x2813da900) [active]> that is not interruptible cannot be stopped!'
*** First throw call stack:
(0x1c1b95e88 0x1baebf8d8 0x1bc4adb4c 0x1c420e334 0x1033c4440 0x1033cae6c 0x1ca361754 0x1ca3610a8 0x1ca38d0d4 0x1ca373cbc 0x1ca35beac 0x1bc050b90 0x1bc0555bc 0x1bc0a4524 0x1bc0a7a08 0x1bc0a3d54 0x1bbfa43f0 0x1bbfba8d4 0x1bbfa7860 0x1bbfa7590 0x1bc002d24 0x1c3e0db2c 0x1c3f80c28 0x1c3d8eaa8 0x1c3d8e638 0x1c3d8c648 0x1c3d88360 0x1c3e4e514 0x1c3e4e434 0x1c4391ec0 0x1c3ec4a0c 0x1c3fe29ac 0x1c3e2699c 0x1c3dd4e68 0x1c3dd432c 0x1c3dd40c0 0x1c3d4e020 0x1c3da79d0 0x1c32239ec 0x1c3e1b1dc 0x1c3eefaf4 0x1c3eef834 0x1c3ebeddc 0x1c3e2be20 0x1c42bcb8c 0x1c42b67dc 0x1c41bd07c 0x1c422d9cc 0x1c42b3700 0x1c409d5cc 0x1c409cee8 0x1c409ca8c 0x1c3e28880 0x1c3f66d88 0x1c40f9800 0x1c41276f0 0x1c3e191e4 0x1c3dec664 0x1c40178f0 0x1c401769c 0x1c401733c 0x1c40165f8 0x1c4a9f4e8 0x1c4015fa8 0x1c3fefdd0 0x1c4177520 0x1c3ff17b4 0x1c4bee550 0x1c4bee698 0x1c404bffc 0x1c3e191e4 0x1c3e49958 0x1c46bbed0 0x1c4754f98 0x1c3e867a8 0x1c3e855e0 0x1c42f87a8 0x1c3f580b8 0x1c3f57f28 0x1c3f57d68 0x1d77b1cdc 0x1d77b1b18 0x1d77b5294 0x1d77b51b0 0x1084b205c 0x1084b5ad8 0x1d77bf3b0 0x1d77bef4c 0x1d77c172c 0x1c1c61f54 0x1c1c6e32c 0x1c1bf2210 0x1c1c07ba8 0x1c1c0ced4 0x1faf0e368 0x1c40eb3d0 0x1c40eb034 0x1034868bc 0x1e0274960)
libc++abi: terminating with uncaught exception of type NSException
*** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'An animator <UIViewPropertyAnimator(0x2813da900) [active]> that is not interruptible cannot be stopped!'
terminating with uncaught exception of type NSException
(lldb) 

Tested with device iPhone 13 Pro 16.1, Crash in 00:17

RPReplay_Final1669983404.MP4

@koke
Copy link
Member Author

koke commented Dec 2, 2022

Good catch, I pushed a couple new commits that should make this more solid:

  • We only observe changes to the navbar height when the view is visible
  • When the view becomes visible, instead of updating the state through the .initial option in the observer, we update manually from viewDidAppear so that we can specify that we don't want to animate that update
  • Reorganize the code better to allow animation to be optional

I tested a few random scenarios and it seems to work fine. A small glitch is that if you leave the screen while you are on portrait with the header visible, rotate to landscape from other screen, and go back to My Store, the header will be visible for a split second before disappearing. This is because I'm running the update from viewDidAppear instead of viewWillAppear, but when I tried that it didn't work since the navbar was being reported as 96px tall for some reason 🤷🏽

@peril-woocommerce
Copy link

Warnings
⚠️ This PR is assigned to a milestone which is closing in less than 2 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

@koke koke modified the milestones: 11.5, 11.6 Dec 2, 2022
@koke
Copy link
Member Author

koke commented Dec 2, 2022

Moving this to 11.6, if you get to it earlier, feel free to move back to 11.5 and merge

@toupper
Copy link
Contributor

toupper commented Dec 5, 2022

Now it tests well @koke, thanks for the change. I just added a non-blocker comment. :shipit:

///
func updateAnnouncementCardVisibility() {
announcementView?.isHidden = navigationBarIsShort
func showHeader() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocker but perhaps we could collapse these two methods that look very similar into one with a parameter:

func changeHeaderVisibility(isHidden: Bool) {
        contentTopToHeaderConstraint?.isActive = !isHidden
        headerStackView.alpha = isHidden ? 1 : 0
        view.layoutIfNeeded()
}

Copy link
Member Author

Choose a reason for hiding this comment

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

On one hand, I like the idea of reducing duplication, but I think I lean towards the current implementation since it seems clearer what showing or hiding would do, without having to parse inline conditionals.

@koke koke merged commit 3332b19 into trunk Dec 5, 2022
@koke koke deleted the try/jitm-animate-take-2 branch December 5, 2022 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature: jitm Related to Just In Time Messages type: enhancement A request for an enhancement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Just In Time Messages] JITM UI improvements on the My Store screen

4 participants