[Just in Time Messages] Add Modal JITM template support for Tap to Pay campaign #9694
Conversation
Also prep JITM view model for multiple-display use
Temporarily use IPP modals to show a JITM
joshheald
added
type: enhancement
joshheald
changed the title
You can test the changes from this Pull Request by:
|
Because of the additional delay in dismissing the modal JITM, universal links to Set up Tap to Pay on iPhone only got as far as the payments menu. This was due to using a short async delay to trigger the navigation to the Set up TTPoI flow – which works with a banner JITM, but triggers before the screen is presented when called from a modal JITM, so `navigationController` was still nil. That meant there was nothing to show the Set up TTPoI flow modal from. I’ve added a callback in the `viewDidLoad` for the view controller which allows us to trigger the navigation step at the right time, i.e. when the menu screen is fully loaded.
Generated by 🚫 dangerJS |
|
👋 I have started to check into this but I haven't got into the code yet, during testing I found 2 cases that I cannot replicate, so I'm not sure if I'm following the instructions correctly: ✅ Functionality: Deeplinking to Set up TTP Click me❌ Appearance: Removing the image urls from the response – observe that the default image is used: Removing the URLs from the response does not show the default images: Click meWe also seem to have a failing test: |
|
Thanks for looking at it @iamgabrielma ... I'll try to help with what's going on but... it works on my machine 😬 webview-and-default-image.mp4
Your JSON looks correct to me. When I set the link to the M2 product page in my sandbox, tapping the button dismisses the modal and shows the web view. Here's some JSON which I've copied from a response my sandbox made to the app, so used the actual rule definition:
This time your JSON doesn't look like what I intended, but should still work. I wasn't specific enough here, really I meant for you to remove the I've tried with both cases though, and it shows the megaphone image as per the default when I do it. This has made me think a bit though: we should probably show no image in this case, because it's less decorative and more part of the content here. Showing a JITM modal without an image seems like a valid thing to want to do, so I'll change that behaviour... though perhaps not on this PR. Two thoughts:
Argh sorry. I'll sort that. I think we probably need to bump this to 13.7 at this point |
There was a problem hiding this comment.
Is there any chance you're modifying the wrong sequenced response in Proxyman/Charles? I.e. could you check with an app breakpoint as well that the app is getting the network response you intend it to be?
It is possible, today when I tried again it didn't work the first time but then started working for all my other tests. The response I used is the same, so perhaps due to Proxyman configuration, as I had to reconfigure the certificates and such. It "works on my machine" now as well 🤷🏻♂️
Could you check for any console log messages about displaying a modal while something else is already presented? That is a risk I thought I'd covered with a recent commit, but it could be stopping the web view modal from showing if I... didn't.
That seems to be the issue, I've written down the error in the comment here, with some attempted (but ultimately failed) fix. IMHO this detail can be handled on a different PR if needed 🚢
| extension JustInTimeMessageTemplate { | ||
| /// Returns a "ready to use" type filled with fake values. | ||
| /// | ||
| public static func fake() -> JustInTimeMessageTemplate { | ||
| .banner | ||
| } | ||
| } |
There was a problem hiding this comment.
Much cleaner for tests 💯
| } | ||
|
|
||
|
|
||
| extension JustInTimeMessageViewModel: AnnouncementCardViewModelProtocol { |
There was a problem hiding this comment.
TIL that we cannot make this extension's access control private because is declaring protocol conformances, it makes sense, but I didn't realized until the compiler told me :D
| self.modalJustInTimeMessageHostingController = modalController | ||
| modalController.view.backgroundColor = .clear | ||
| modalController.modalPresentationStyle = .overFullScreen | ||
| self.present(modalController, animated: true) |
There was a problem hiding this comment.
As you guessed on #9694 (comment), there's indeed an error logged in the console about attempting to present the web view when we're already presenting the JITM UIKit wrapper:
WooCommerce[59729:46069113] [Presentation] Attempt to present <_TtGC7SwiftUI19UIHostingControllerV11WooCommerce12WebViewSheet_: 0x12d101000> on <WooCommerce.MainTabBarController: 0x127879e00> (from <WooCommerce.DashboardViewController: 0x12c05da00>) which is already presenting <_TtGC11WooCommerce36ConstraintsUpdatingHostingControllerVS_28JustInTimeMessageModal_UIKit_: 0x12c0b9600>.
I did a quick experiment by switching .present(_:) to .show(_:) instead, so the system handles the navigation:
| self.present(modalController, animated: true) | |
| self.show(modalController, sender: self) |
This fixes the dismissal and redirection issue to what we expect, however the overlay is still in the middle, so definitely not a final fix 😅 :
There was a problem hiding this comment.
I don't really want to change the presentation style for this... the JITM modal already dismisses itself when the CTA is tapped, but I've added a second call to do that from the DashboardViewController right before the web view is presented as an extra check.
N.B. I couldn't reproduce this issue .
| #if DEBUG // to avoid importing things we don't need in the implementation | ||
| import Yosemite |
There was a problem hiding this comment.
That's a nice way to deal with importing the module just for the preview 💯
| @unknown default: | ||
| EmptyView() |
There was a problem hiding this comment.
Should we add a log, or track this case somehow? Otherwise we might fail to catch on time new AdaptiveAsyncImage cases when/if are introduced
There was a problem hiding this comment.
No, because:
- we can't do that in the middle of a
View.body - we don't need to, because
@unknown defaultmeans the compiler will still produce a warning for any unhandled cases. So if another case gets added in future, we'll know about it when we build the app.
| Color.black.opacity(0.5) | ||
| .edgesIgnoringSafeArea(.all) | ||
| .onTapGesture { | ||
| dismiss() | ||
| } | ||
| .transition(.opacity) | ||
| .animation(.easeInOut(duration: 0.1), value: internalIsPresented) | ||
|
|
||
| GeometryReader { geometry in | ||
| VStack { | ||
| content() | ||
| .padding(16) | ||
| .frame(width: geometry.size.width * 0.75) | ||
| .frame(maxHeight: geometry.size.height * 0.8) | ||
| .fixedSize(horizontal: false, vertical: true) // these three modifiers define the container size | ||
| .background(Color(.tertiarySystemBackground)) | ||
| .cornerRadius(10) | ||
| .shadow(radius: 10) | ||
| } | ||
| .frame(maxWidth: .infinity, maxHeight: .infinity, alignment: .center) // Ensures the container is centred | ||
| } | ||
| .transition(.move(edge: .bottom)) | ||
| .animation(.easeInOut(duration: 0.25), value: internalIsPresented) |
There was a problem hiding this comment.
Should we move the numbers to a Constants extension?
There was a problem hiding this comment.
This is one of those places where I feel like it would make the code less readable... and there's no (real) repetition, because the two 10s wouldn't be the same constant anyway.
If a modal JITM is showing when we open the webview, we’ll get an error and the webview won’t open. This shouldn’t really happen, because the JITM modal already dismisses itself when we tap its CTA, but just in case we can do a pre-emptive dismissal before showing the webview.
Closes: #9679
Description
Apologies that this is slightly long. If it would help, I could split the
ModalOverlayfrom theJustInTimeMessageModal, however the overlay would not be easily testable.We want to show a modal-style Just in Time Message, for a higher-visibility message. This PR adds a template, and modal overlay display style, to provide that messaging.
Tapping the CTA navigates to a web view or universal link in the same way as banner JITMs do
Tapping the
Maybe laterbutton dismisses the JITM modal, and logs the dismissal with the JITM systemTapping outside the modal temporarily dismisses the JITM modal, but does not log the dismissal with the JITM system.
N.B. – on the My Store screen, store onboarding has priority over Just in Time Messages. Originally this was because they used the same space, and getting a store set up fully would generally come before IPP-related actions, which is what JITMs were/are used for. This restriction continues, and still applies to modal JITMs even though they don't share space.
Testing instructions
Test setup for this is non-trivial until the JITM message definitions are added - sorry! There are two options:
In either case, you'll also need to host the images somewhere, and use the correct conventions when adding them to the JITM definition/JSON response.
Using a proxy
Using Charles or Proxyman, set a breakpoint on the following endpoint pattern:
https://public-api.wordpress.com/rest/v1.1/jetpack-blogs/*/rest-api/?json=true&locale=*&path=/jetpack/v4/jitm*Modify the response JSON using the following sample JSON response:
Using a sandbox
A sample JITM definition (for the sandbox option) is below:
public_html/wp-content/mu-plugins/0-sandbox.phpImage hosting – I uploaded the images to my ephemeral site's media library, and used their URLs in the definition/response. Replace
{EPHEMERAL-SITE-HOST}with your site's domain if you follow the same route. While it remains up, you're welcome to usepale-pirate-peach.atomicsites.blogif you like, just to save a step of hosting the images.Testing in the app
Whichever route you choose:
Functionality: Deeplinking to Set up TTP
Functionality: Dismissal
Maybe laterbutton – observe that the modal is dismissedFunctionality: Opening a web view
Change the network response/JITM definition so that the CTA URL is
https://woocommerce.com/products/hardware/usAppearance
Screenshots
JITM.modal.mp4
RELEASE-NOTES.txtif necessary.