[Just In Time Messages] Display Just In Time Message on the My Store screen (visual UI only)

joshheald commented 1 year ago

Part of: #7916

Description

In order to promote In-Person Payments to relevant merchants, we are adding JITM support in the mobile apps. In the first instance, we will support JITM support on the My Store screen, above the analytics charts.

In this PR, we display the first JITM if any are returned. We se content.message as the title, content.description as the detail, and CTA.message as the button title. The JITM is displayed by reusing the FeatureAnnouncementCard UI.

Interplay with Product Onboarding

There is another project, Product Onboarding, which will use the same space and UI to display a message encouraging merchants to add their first product. See pe5pgL-dX-p2 for full context. This won't be a JITM, and will take precedence over any JITM, at least in the initial implementation.

@rachelmcr has confirmed that this approach looks feasible for the Product Onboarding project.

Refactoring

Since we're using the same UI in the same place, I've added the JITM UI in a way that it can be easily used by the product onboarding instead. See DashboardViewModel.syncAnnouncements(), adding code to publish, for example, a ProductOnboardingAnnouncementCardViewModel, will show the onboarding card.

To achieve this, I've done a partial refactor of the FeatureAnnouncementCard, to have it use a new AnnouncementCardViewModelProtocol which can be adopted by any view model we need. There is more which could be factored out of this protocol, but that's an exercise for future improvements.

I've also removed the FeatureAnnouncementCardCell, as it was tricky to make the changes there, and unnecessary in light of the HostingTableViewCell<FeatureAnnouncementView> which I've used in its place. The cell was only used in one place, for the Product Form: after adding a new product, the announcement card is shown to promote setting product upsells and cross-sells.

I've tested that the upsell campaign still works with the new cell. @ealeksandrov – I think you initially implemented the cell I removed here: feel free to take a look over the code as well if you like, but no worries if you've not got the time just now.

Testing instructions

JITM on my store

N.B. 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
Open the app
Observe that the message is shown

Note that dismissal and CTA buttons are not yet functional.

The banner is behind a feature flag: try with a Release build, to make sure that the banner does not show and the network request to /jetpack/v4/jitm is not made.

Existing product upsell/cross-sell campaign still works after removal of cell

Screenshots

JITM on My Store

My Store screen with a JITM visible

Product upsell/cross-sell campaign still works after removal of cell

https://user-images.githubusercontent.com/2472348/198277688-02702915-00f7-429c-bc17-9912cee57e01.mp4

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

wpmobilebot commented 1 year ago

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 pr7948-7da028c on your iPhone

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

joshheald commented 1 year ago

@rachelmcr Just adding you as an optional reviewer, as mentioned. No changes since I sent this over before.

The UI appearing is a bit jerky at the moment, I'll look at animating its insertion in a future PR.

ealeksandrov commented 1 year ago

Thanks for cleaning up FeatureAnnouncementCardCell! Unfortunately when testing this PR I still see UI bug that forced me to fallback on UIKit:

https://user-images.githubusercontent.com/3132438/198286294-cbea3b13-4815-4cac-8afd-b082f7bd2f60.mp4

It would be nice to unify all announcement views but I haven't found a fix for this glitch. Happens on iOS 15 and 16, simulator and device.

toupper commented 1 year ago

@joshheald The code looks great, but I found a couple of UI glitches that need to be fixed in my opinion:

When changing orientation to landscape and back to portrait, there's an extra top space above the message:

Each time we navigate to another tab and back to My Store tab the message is added again:

pachlava commented 1 year ago

@joshheald 👋

Apart from issues mentioned by @toupper, I've noticed only one thing:

I think that black background colour makes the message break the design consistency a bit. Possibly, making it grey (as it's done for upsell banner in Orders List) will improve the situation.

joshheald commented 1 year ago

@toupper thank you for the speedy review!

I found a couple of UI glitches that need to be fixed in my opinion:

When changing orientation to landscape and back to portrait, there's an extra top space above the message:

Each time we navigate to another tab and back to My Store tab the message is added again.

Multiple cards

The second of these was really two bugs: I requested JITMs in viewWillAppear, and I didn't properly remove existing cards from display before showing a new one.

These are fixed in c91c2c2 and c3c7d0e respectively.

Orientation change

This looks like it is iOS 15 only. However, I don't really think it works well in landscape, because it doesn't scroll away with it being in the header.

I think we should hide it in landscape, and make it collapse like the title does, when we scroll in portrait.

joshheald commented 1 year ago

@pachlava thanks!

I think that black background colour makes the message break the design consistency a bit. Possibly, making it grey (as it's done for upsell banner in Orders List) will improve the situation.

I agree. Fixed in 85097b4

dark-mode

joshheald commented 1 year ago

@ealeksandrov Thanks for checking this, and sorry I didn't spot the scrolling issue!

Unfortunately when testing this PR I still see UI bug that forced me to fallback on UIKit:

It would be nice to unify all announcement views but I haven't found a fix for this glitch. Happens on iOS 15 and 16, simulator and device.

It certainly would, what a shame that it doesn't work. I tried a bunch of stuff to fix it too:

Ruled out the containing stack view being the issue
Tried invalidateIntrinsicContentSize() in the host cell, instead of layoutIfNeeded() as per this article
Messed with all the constraints on the host cell and the stack in the card View.
Other stuff that I can't remember now...

No luck though. I've reverted the removal of the UITableViewCell subclass in 7b89fb2, and tested it again, the Linked Products promo works as it did before.

joshheald commented 1 year ago

Orientation change

This looks like it is iOS 15 only. However, I don't really think it works well in landscape, because it doesn't scroll away with it being in the header.

I think we should hide it in landscape, and make it collapse like the title does, when we scroll in portrait.

@toupper this is resolved in aa6e6ee

https://user-images.githubusercontent.com/2472348/198359086-9d83e61f-1c97-4653-a33a-2be3001f1f5b.mp4

Please could you re-review?

I'm also going to see if this hosting controller fixes the cell issue...

joshheald commented 1 year ago

I'm also going to see if this hosting controller fixes the cell issue...

... no such luck 😞

toupper commented 1 year ago

The latest commits fixed the issues I mentioned, LGTM! :shipit:

woocommerce / woocommerce-ios