[Woo POS] M2: Exit POS modal presentation #13530

bozidarsevo · 2024-08-07T13:29:27Z

Description

As a part of M2, info modal for exiting POS is added.
When user taps the three dot button in floating action control and taps "Exit POS" we present this info modal where user had the option to either exit POS or close the modal.
This way we are adding one more step of confirmation to make sure users to not exit POS by accident.

Testing information

Enter POS
Tap three dot button in floating action control
Check that the Exit POS modal is shown
Tap on the X button and check that the modal is dismissed but you are still in POS
Tap three dot button in floating action control again
Tap on the "Exit" button and check that you have exited POS successfully

Screenshots

Simulator.Screen.Recording.-.iPad.Pro.11-inch.4th.generation.17.2.-.2024-08-07.at.15.29.05.mp4

I have considered adding unit tests for this change. If I decided not to add them, I have provided a brief explanation below (optional):
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.
This PR includes refactoring; smoke testing of the entire section is needed.

bozidarsevo · 2024-08-07T13:30:06Z

WooCommerce/Classes/POS/Presentation/PointOfSaleAssets.swift

@@ -9,7 +9,7 @@ enum PointOfSaleAssets: CaseIterable {
    case exit
    case getSupport
    case removeCartItem
-    case dismissProductsBanner
+    case xClose


Renamed this since it is being used in various places so I wanted to make it a bit more generic

We spoke about with @joshheald offline but it would make sense to use SF symbols rather than images for most of these. xmark represents the same symbol, so we could use Image(systemName: "xmark").

dangermattic · 2024-08-07T13:30:08Z

	1 Warning
⚠️	This PR is assigned to the milestone 19.9. This milestone is due in less than 2 days. Please make sure to get it merged by then or assign it to a milestone with a later deadline.

Generated by 🚫 Danger

bozidarsevo · 2024-08-07T13:30:36Z

WooCommerce/Classes/POS/Presentation/PointOfSaleDashboardView.swift

+                    .padding(.bottom)
+                    .padding(.bottom)
+                    .padding(.bottom)


Is this ok? I was trying to avoid writing explicit numbers for padding but wanted to make it be like on Figma designs.

I think it's fine to use an explicit number for padding.

You can define it in one of the shared Layout structs, or the Constants enum for this view

bozidarsevo · 2024-08-07T13:33:49Z

WooCommerce/Classes/POS/Presentation/Reusable Views/POSModalViewModifier.swift

+    let fixedWidth: Bool
+    let fixedHeight: Bool


Added this properties to allow more flexible usage/layout of the POS modals

wpmobilebot · 2024-08-07T13:40:06Z

📲 You can test the changes from this Pull Request in WooCommerce iOS by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce iOS
	Build Number	pr13530-358d15c
	Version	19.8
	Bundle ID	com.automattic.alpha.woocommerce
	Commit	`358d15c`
	App Center Build	WooCommerce - Prototype Builds #10297

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

staskus

Thanks, looks good!

I left a couple of suggestions 👍

staskus · 2024-08-07T16:48:08Z

WooCommerce/Classes/POS/Presentation/PointOfSaleAssets.swift

@@ -9,7 +9,7 @@ enum PointOfSaleAssets: CaseIterable {
    case exit
    case getSupport
    case removeCartItem
-    case dismissProductsBanner
+    case xClose


We spoke about with @joshheald offline but it would make sense to use SF symbols rather than images for most of these. xmark represents the same symbol, so we could use Image(systemName: "xmark").

staskus · 2024-08-07T16:49:38Z

WooCommerce/Classes/POS/Presentation/PointOfSaleDashboardView.swift

@@ -59,6 +59,37 @@ struct PointOfSaleDashboardView: View {
                }
            }
        })
+        .posModal(isPresented: $viewModel.showExitPOSModal, fixedWidth: false, fixedHeight: false, content: {
+            VStack(spacing: 0 ) {


Lets move it to a separate method or a file just as we have with PointOfSaleCardPresentPaymentAlert to keep this method cleaner.

staskus · 2024-08-07T16:50:03Z

WooCommerce/Classes/POS/Presentation/PointOfSaleDashboardView.swift

+                            .foregroundColor(Color.posTertiaryTexti3)
+                    }
+                }
+                Text("Exit Point of Sale mode?")


Localized string

staskus · 2024-08-07T16:52:21Z

WooCommerce/Classes/POS/Utils/Font+WooCommercePOS.swift

@@ -3,4 +3,6 @@ import SwiftUI
 extension Font {
    static let posBody: Font = Font.system(size: UIFontMetrics.default.scaledValue(for: 24))
    static let posTitle: Font = Font.largeTitle
+    static let posModalBody: Font = Font.system(size: UIFontMetrics.default.scaledValue(for: 24))


These are the same as posBody and posTitle TfaZ4LUkEwEGrxfnEFzvJj-fi-3385_18076

We have a task to clearly define all the fonts from specification and maybe we should do it sooner rather than later. For this PR I suggest using the existing font definitions.

staskus · 2024-08-07T16:53:32Z

WooCommerce/Classes/POS/Presentation/PointOfSaleDashboardView.swift

+                    .padding(.bottom)
+                    .padding(.bottom)
+                    .padding(.bottom)


I think it's fine to use an explicit number for padding.

joshheald

@bozidarsevo Some suggested changes – main one is the removal of fixedHeight: false for the connection modals, so they stay a consistent size throughout.

joshheald · 2024-08-08T08:09:59Z

WooCommerce/Classes/POS/Presentation/PointOfSaleAssets.swift

@@ -9,7 +9,7 @@ enum PointOfSaleAssets: CaseIterable {
    case exit
    case getSupport
    case removeCartItem
-    case dismissProductsBanner
+    case xClose


joshheald · 2024-08-08T08:11:08Z

WooCommerce/Classes/POS/Presentation/PointOfSaleDashboardView.swift

+                    .padding(.bottom)
+                    .padding(.bottom)
+                    .padding(.bottom)


You can define it in one of the shared Layout structs, or the Constants enum for this view

joshheald · 2024-08-08T08:15:01Z

WooCommerce/Classes/POS/Presentation/PointOfSaleDashboardView.swift

@@ -46,7 +46,7 @@ struct PointOfSaleDashboardView: View {
        .animation(.easeInOut, value: viewModel.isInitialLoading)
        .background(Color.posBackgroundGreyi3)
        .navigationBarBackButtonHidden(true)
-        .posModal(isPresented: $totalsViewModel.showsCardReaderSheet, content: {
+        .posModal(isPresented: $totalsViewModel.showsCardReaderSheet, fixedHeight: false, content: {


Suggested change

.posModal(isPresented: $totalsViewModel.showsCardReaderSheet, fixedHeight: false, content: {

.posModal(isPresented: $totalsViewModel.showsCardReaderSheet, content: {

Height should be fixed – otherwise the card reader connection modal changes size as the connection progresses, which is quite distracting.

joshheald · 2024-08-08T08:21:56Z

WooCommerce/Classes/POS/Presentation/PointOfSaleDashboardView.swift

+                HStack {
+                    Spacer()
+                    Button {
+                        viewModel.showExitPOSModal = false
+                    } label: {
+                        Image(PointOfSaleAssets.xClose.imageName)
+                            .frame(width: Constants.closeIconSize, height: Constants.closeIconSize)
+                            .foregroundColor(Color.posTertiaryTexti3)


You could consider putting all this in a
.overlay(alignment: .topTrailing) modifier – see FeatureAnnouncementCardView for an example

joshheald · 2024-08-08T08:27:06Z

WooCommerce/Classes/POS/Presentation/PointOfSaleDashboardView.swift

+                    .padding(.bottom)
+                    .padding(.bottom)
+                Button {
+                    presentationMode.wrappedValue.dismiss()


Suggested change

presentationMode.wrappedValue.dismiss()

dismiss()

This is pre-existing, but it would be nice to shift to using:

@Environment(\.dismiss) private var dismiss

for dismissal, as it's a bit more readable.

joshheald

I made a change so it just needs the binding to dismiss the modal, not the whole vm.

joshheald · 2024-08-09T05:32:49Z

WooCommerce/Classes/POS/Presentation/PointOfSaleExitPosAlertView.swift

+    @ObservedObject private var viewModel: PointOfSaleDashboardViewModel
+
+    init(viewModel: PointOfSaleDashboardViewModel) {
+        self.viewModel = viewModel
+    }


It would be nice if this just had the isPresented binding, not the whole view model

Done in 358d15c

Yep, good point, tnx!

bozidarsevo added 4 commits August 7, 2024 15:21

Rename icon to xClose

02ae6d3

Update Font+WooCommercePOS.swift

5240dad

Update PointOfSaleDashboardViewModel.swift

6cb21d9

Add exit POS modal

047c059

bozidarsevo added the feature: POS label Aug 7, 2024

bozidarsevo added this to the 19.9 milestone Aug 7, 2024

bozidarsevo commented Aug 7, 2024

View reviewed changes

bozidarsevo marked this pull request as ready for review August 7, 2024 14:00

Merge branch 'trunk' into issue/13278-exit-pos-modal

212a98b

bozidarsevo requested review from joshheald and staskus August 7, 2024 15:13

staskus reviewed Aug 7, 2024

View reviewed changes

joshheald approved these changes Aug 8, 2024

View reviewed changes

joshheald mentioned this pull request Aug 8, 2024

[Woo POS] Simple products information modal #13533

Merged

3 tasks

bozidarsevo added 5 commits August 8, 2024 16:26

Merge branch 'trunk' into issue/13278-exit-pos-modal

57e7cd6

Revert and update close icon

447abe6

Update PointOfSaleDashboardView.swift

d45868d

Create PointOfSaleExitPosAlertView

921e982

Added Constants

2862373

bozidarsevo requested review from joshheald and staskus August 8, 2024 15:00

13278 Pass the binding only for dismissal

358d15c

joshheald approved these changes Aug 9, 2024

View reviewed changes

bozidarsevo merged commit 2a72a74 into trunk Aug 9, 2024
22 checks passed

bozidarsevo deleted the issue/13278-exit-pos-modal branch August 9, 2024 08:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Woo POS] M2: Exit POS modal presentation #13530

[Woo POS] M2: Exit POS modal presentation #13530

bozidarsevo commented Aug 7, 2024 •

edited

Loading

bozidarsevo Aug 7, 2024

staskus Aug 7, 2024

joshheald Aug 8, 2024

dangermattic commented Aug 7, 2024 •

edited

Loading

bozidarsevo Aug 7, 2024

staskus Aug 7, 2024

joshheald Aug 8, 2024

bozidarsevo Aug 7, 2024

wpmobilebot commented Aug 7, 2024 •

edited

Loading

staskus left a comment

staskus Aug 7, 2024

staskus Aug 7, 2024

joshheald Aug 8, 2024

staskus Aug 7, 2024

staskus Aug 7, 2024

staskus Aug 7, 2024

joshheald left a comment

joshheald Aug 8, 2024

joshheald Aug 8, 2024

joshheald Aug 8, 2024

joshheald Aug 8, 2024

joshheald Aug 8, 2024

joshheald left a comment

joshheald Aug 9, 2024

joshheald Aug 9, 2024

bozidarsevo Aug 9, 2024

	.posModal(isPresented: $totalsViewModel.showsCardReaderSheet, fixedHeight: false, content: {
	.posModal(isPresented: $totalsViewModel.showsCardReaderSheet, content: {

[Woo POS] M2: Exit POS modal presentation #13530

[Woo POS] M2: Exit POS modal presentation #13530

Conversation

bozidarsevo commented Aug 7, 2024 • edited Loading

Description

Testing information

Screenshots

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dangermattic commented Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wpmobilebot commented Aug 7, 2024 • edited Loading

staskus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshheald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshheald left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bozidarsevo commented Aug 7, 2024 •

edited

Loading

dangermattic commented Aug 7, 2024 •

edited

Loading

wpmobilebot commented Aug 7, 2024 •

edited

Loading