-
Notifications
You must be signed in to change notification settings - Fork 110
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Woo POS] M2: Exit POS modal presentation #13530
Conversation
@@ -9,7 +9,7 @@ enum PointOfSaleAssets: CaseIterable { | |||
case exit | |||
case getSupport | |||
case removeCartItem | |||
case dismissProductsBanner | |||
case xClose |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed this since it is being used in various places so I wanted to make it a bit more generic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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")
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #13458
.padding(.bottom) | ||
.padding(.bottom) | ||
.padding(.bottom) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this ok? I was trying to avoid writing explicit numbers for padding but wanted to make it be like on Figma designs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine to use an explicit number for padding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can define it in one of the shared Layout structs, or the Constants
enum for this view
let fixedWidth: Bool | ||
let fixedHeight: Bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this properties to allow more flexible usage/layout of the POS modals
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good!
I left a couple of suggestions 👍
@@ -9,7 +9,7 @@ enum PointOfSaleAssets: CaseIterable { | |||
case exit | |||
case getSupport | |||
case removeCartItem | |||
case dismissProductsBanner | |||
case xClose |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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")
.
@@ -59,6 +59,37 @@ struct PointOfSaleDashboardView: View { | |||
} | |||
} | |||
}) | |||
.posModal(isPresented: $viewModel.showExitPOSModal, fixedWidth: false, fixedHeight: false, content: { | |||
VStack(spacing: 0 ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets move it to a separate method or a file just as we have with PointOfSaleCardPresentPaymentAlert
to keep this method cleaner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
➕
.foregroundColor(Color.posTertiaryTexti3) | ||
} | ||
} | ||
Text("Exit Point of Sale mode?") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Localized string
@@ -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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
.padding(.bottom) | ||
.padding(.bottom) | ||
.padding(.bottom) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine to use an explicit number for padding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bozidarsevo Some suggested changes – main one is the removal of fixedHeight: false
for the connection modals, so they stay a consistent size throughout.
@@ -9,7 +9,7 @@ enum PointOfSaleAssets: CaseIterable { | |||
case exit | |||
case getSupport | |||
case removeCartItem | |||
case dismissProductsBanner | |||
case xClose |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #13458
.padding(.bottom) | ||
.padding(.bottom) | ||
.padding(.bottom) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can define it in one of the shared Layout structs, or the Constants
enum for this view
@@ -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: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.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.
HStack { | ||
Spacer() | ||
Button { | ||
viewModel.showExitPOSModal = false | ||
} label: { | ||
Image(PointOfSaleAssets.xClose.imageName) | ||
.frame(width: Constants.closeIconSize, height: Constants.closeIconSize) | ||
.foregroundColor(Color.posTertiaryTexti3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could consider putting all this in a
.overlay(alignment: .topTrailing)
modifier – see FeatureAnnouncementCardView
for an example
.padding(.bottom) | ||
.padding(.bottom) | ||
Button { | ||
presentationMode.wrappedValue.dismiss() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a change so it just needs the binding to dismiss the modal, not the whole vm.
@ObservedObject private var viewModel: PointOfSaleDashboardViewModel | ||
|
||
init(viewModel: PointOfSaleDashboardViewModel) { | ||
self.viewModel = viewModel | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if this just had the isPresented
binding, not the whole view model
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 358d15c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, good point, tnx!
Closes: #13278
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
Screenshots
Simulator.Screen.Recording.-.iPad.Pro.11-inch.4th.generation.17.2.-.2024-08-07.at.15.29.05.mp4
RELEASE-NOTES.txt
if necessary.