[IPPInAppFeedback] Orders screen banner placement #8640

iamgabrielma · 2023-01-16T04:46:38Z

Closes: #8636
Part of: #8530

Description

This PR adds the in-app feedback banner to the Order view, as well as the buttons to launch a survey and dismiss the banner.

At the moment the banner is not dismissible. Dismiss will be part of a different PR.

Changes

Adds a .IPP case as new FeedbackType
Upon configuring OrderListViewController, we set the existing top banner view (generally used to display the Order Creation banner, or an error) to a new IPP banner survey. This uses the existing TopBannerView and TopBannerViewModel components.
We have a new Publisher hideIPPFeedbackBanner, so we can listen to changes and set priorities on the different banners that should display in the view.
The banner has an ActionButton that will launch the feedback survey via the existing SurveyCoordinatingController component.

Testing instructions

Go to Menu > Orders > See the banner at the top of the screen
Tap "Share Feedback", a WebView showing a survey form will shop up. Scroll down and tap "Finish Survey", a "Feedback Sent" view will be shown and you'll be redirected to the Orders page.

Screenshots

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

wpmobilebot · 2023-01-16T05:01:04Z

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 pr8640-1190434 on your iPhone

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

toupper · 2023-01-16T10:16:37Z

Great job @iamgabrielma! I added a few comments regarding the code.
Apart from that, I have a question about the design. It seems like the banner message -"Rate your In-Person Payments experience"- does not go under the x button in the original design, unlike in your implementation:

Is that something we have to fix?

toupper · 2023-01-16T09:46:48Z

Storage/Storage/Model/FeedbackType.swift

+
+    /// Identifier for the In-Person Payments feedback survey
+    ///
+    case IPPFeedback


Minor thing here, if the enum type name already contains "Feedback", I think we can omit it here: case IPP, as in the case of the ordersCreation

Changed on 83835c4

toupper · 2023-01-16T09:49:20Z

WooCommerce/Classes/ViewRelated/Orders/OrderListViewModel.swift

+
+        if ServiceLocator.featureFlagService.isFeatureFlagEnabled(.IPPInAppFeedbackBanner) {
+            // TODO: Remove. Temporarily set to false so we can debug the IPPFeedback banner easily.
+            hideOrdersBanners = false


I find a bit dangerous to keep this in the PR, even if we add the TODO comment. If it goes to trunk it would disable the banner, which is not what we want

You're right, that makes sense. I'll experiment with the @Published properties to find a better way to display/hide these.

I see we're using Publishers.CombineLatest(:) so the view model internal state figures out which banner should assign to the view, we could pass the $hideIPPFeedbackBanner binding as well there and use CombineLatest3(:) to figure this one out.

toupper · 2023-01-16T09:52:44Z

WooCommerce/Classes/ViewRelated/Orders/OrderListViewModel.swift

-        loadOrdersBannerVisibility()

        if ServiceLocator.featureFlagService.isFeatureFlagEnabled(.IPPInAppFeedbackBanner) {
+            loadIPPFeedbackBannerVisibility()


I have a question here, don't we load the orders banner if the IPP Feedback is enabled?

Good question, after thinking further about it I was unsure how to fully deal with the Order Creation banner if IPP is enabled.

As per discussion at p1673883711765819?thread_ts=1673876649.359489&cid=C025A8VV728-slack-C025A8VV728 we're opting for:

Prioritise showing the IPP survey. Show the Order Creation banner only when the IPP survey is not displayed.

If IPP survey is dismissed, ideally we shouldn't show Order Creation immediately, unless that's the default behavior and changing it involves a certain level of complexity (as we're targeting to release this survey as soon as possible, then iterate later if needed).

toupper · 2023-01-16T09:53:31Z

WooCommerce/Classes/ViewRelated/Orders/OrderListViewModel.swift

    }

+    func dismissIPPFeedbackBanner() {
+        print("dismiss tapped")


Can we delete these prints here, as this is likely going to trunk?

Definitely, removed on ff7a3ee

The IPP banner dismiss logic is not part of this PR. Removing for simplicity and to reduce scope.

iamgabrielma · 2023-01-18T04:55:58Z

Thanks for your review @toupper , I addressed the comments and this is ready for review again 👍

I have a question about the design. It seems like the banner message -"Rate your In-Person Payments experience"- does not go under the x button in the original design, unlike in your implementation. Is that something we have to fix?

I missed that one! This UI tweak and others is something I'd leave for a different PR ( tracked here ), as we're using an existing component I wanted to work through its functionality first 👯

toupper · 2023-01-18T09:35:29Z

WooCommerce/Classes/ViewRelated/Orders/OrderListViewModel.swift

+            .map { hasError, hasDismissedIPPFeedbackBanner, hasDismissedOrdersBanners -> TopBanner in

-                if hasError {
+                guard !hasError else {


So beautiful code 😉

toupper · 2023-01-18T09:42:36Z

Great job @iamgabrielma! Impressive how you are progressively tackling more complex features 👏

iamgabrielma added 6 commits January 16, 2023 11:18

Add new IPPFeedback topBannerView to OrderList view

67a0786

Create and configure IPP feedback topBannerView

cf2f861

Make the top banner as per design specs

c43dce1

Add NSLocalizedStrings

2055cfb

Display feedback survey when tapped

48a7f92

Add IPPFeedback as new FeedbackType

19cac93

iamgabrielma added type: enhancement A request for an enhancement. feature: mobile payments Related to mobile payments / card present payments / Woo Payments. status: draft This is a draft, still need more work but can be reviewed and commented if asked. labels Jan 16, 2023

iamgabrielma added this to the 12.0 milestone Jan 16, 2023

iamgabrielma mentioned this pull request Jan 16, 2023

[IPPInAppFeedback] Add IPP in-app feedback survey #8530

Closed

16 tasks

iamgabrielma added 2 commits January 16, 2023 14:44

Add feature flag case to Unit Tests

038b736

Dispatch loadFeedbackVisibility action

5f96815

iamgabrielma marked this pull request as ready for review January 16, 2023 07:37

iamgabrielma requested a review from toupper January 16, 2023 07:37

iamgabrielma added status: do not merge Dependent on another PR, ready for review but not ready for merge. and removed status: draft This is a draft, still need more work but can be reviewed and commented if asked. labels Jan 16, 2023

toupper reviewed Jan 16, 2023

View reviewed changes

toupper self-assigned this Jan 16, 2023

iamgabrielma added 2 commits January 16, 2023 21:28

Rename IPP FeedbackType case

83835c4

Remove print statements

ff7a3ee

Base automatically changed from task/ippiaf-eligibility to trunk January 17, 2023 02:03

iamgabrielma mentioned this pull request Jan 17, 2023

[IPPInAppFeedback] Banner dismiss logic #8657

Merged

4 tasks

Add IPP vs Order Creation banner priority

d96ba4e

iamgabrielma removed the status: do not merge Dependent on another PR, ready for review but not ready for merge. label Jan 17, 2023

iamgabrielma added 3 commits January 17, 2023 20:13

Remove dismiss banner logic

b20f591

The IPP banner dismiss logic is not part of this PR. Removing for simplicity and to reduce scope.

Make hideOrdersBanners always true on init

94e63d5

Unit Tests for IPP banner visibility

f0c1d60

iamgabrielma added 2 commits January 18, 2023 10:23

Resolve IPP vs Order Create banner unit tests conflicts

5cbd991

Change initial IPP banner visibility state

cfabb43

iamgabrielma requested a review from toupper January 18, 2023 03:23

Update banner status on each run (for testing)

1190434

iamgabrielma mentioned this pull request Jan 18, 2023

[IPPInAppFeedback] Survey links #8675

Closed

1 task

toupper approved these changes Jan 18, 2023

View reviewed changes

iamgabrielma merged commit a487574 into trunk Jan 18, 2023

iamgabrielma deleted the issue/8636-orders-view-ippiaf-banner branch January 18, 2023 11:43

iamgabrielma mentioned this pull request Jan 23, 2023

[IPPInAppFeedback] UI banner component design tweaks #8728

Merged

[IPPInAppFeedback] Orders screen banner placement #8640

[IPPInAppFeedback] Orders screen banner placement #8640

Uh oh!

Conversation

iamgabrielma commented Jan 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Testing instructions

Screenshots

Uh oh!

wpmobilebot commented Jan 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

toupper commented Jan 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamgabrielma commented Jan 18, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

toupper commented Jan 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

iamgabrielma commented Jan 16, 2023 •

edited

Loading

wpmobilebot commented Jan 16, 2023 •

edited

Loading

toupper commented Jan 16, 2023 •

edited

Loading