-
Notifications
You must be signed in to change notification settings - Fork 121
[POS Settings] Add two-panel settings view #16021
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
Changes from all commits
770cc31
f93dada
239a39b
a2cd1a4
0d3d6bd
ceb4e1b
96edc8e
738ab10
f89e695
d818027
71ade8c
86fe351
cf5d8c7
78b3836
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| import SwiftUI | ||
|
|
||
| struct PointOfSaleSettingsHardwareDetailView: View { | ||
| @State private var navigationPath: [PointOfSaleSettingsView.HardwareDestination] = [] | ||
|
|
||
| var body: some View { | ||
| NavigationStack(path: $navigationPath) { | ||
| List(PointOfSaleSettingsView.HardwareDestination.allCases) { destination in | ||
| NavigationLink(value: destination) { | ||
| HStack(alignment: .firstTextBaseline) { | ||
| Image(systemName: destination.icon) | ||
| .font(.posBodyLargeRegular()) | ||
| VStack(alignment: .leading, spacing: POSPadding.xSmall) { | ||
| Text(destination.title) | ||
| .font(.posBodyLargeRegular()) | ||
| Text(destination.subtitle) | ||
| .font(.posBodyMediumRegular()) | ||
| .foregroundStyle(.secondary) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| .navigationDestination(for: PointOfSaleSettingsView.HardwareDestination.self) { destination in | ||
| VStack(spacing: POSSpacing.medium) { | ||
| Image(systemName: destination.icon).font(.largeTitle) | ||
| .font(.posBodyLargeRegular()) | ||
| Text("\(destination.title) settings") | ||
| .font(.posBodyMediumRegular()) | ||
| Text("Some placeholder") | ||
| .font(.posBodyMediumRegular()) | ||
| .foregroundStyle(.secondary) | ||
| } | ||
| .padding() | ||
| .navigationTitle(destination.title) | ||
| } | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| import SwiftUI | ||
|
|
||
| struct PointOfSaleSettingsHelpDetailView: View { | ||
| var body: some View { | ||
| NavigationStack { | ||
| VStack(alignment: .leading) { | ||
| Text("Help Settings") | ||
| .font(.title2) | ||
| Text("Help-related configuration") | ||
| .font(.caption) | ||
| .foregroundStyle(.secondary) | ||
| } | ||
| .padding() | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| import SwiftUI | ||
|
|
||
| struct PointOfSaleSettingsStoreDetailView: View { | ||
| var body: some View { | ||
| NavigationStack { | ||
| VStack(alignment: .leading) { | ||
| Text("Store Settings") | ||
| .font(.title2) | ||
| Text("Store-related configuration") | ||
| .font(.caption) | ||
| .foregroundStyle(.secondary) | ||
| } | ||
| .padding() | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,223 @@ | ||
| import SwiftUI | ||
|
|
||
| struct PointOfSaleSettingsView: View { | ||
| @Environment(\.dismiss) private var dismiss | ||
| @State private var selection: SidebarNavigation? = .store | ||
|
|
||
| var body: some View { | ||
| POSPageHeaderView( | ||
| title: Localization.navigationTitle, | ||
| trailingContent: { | ||
| Button(action: { dismiss() }) { | ||
| Text(Image(systemName: "xmark")) | ||
| .font(.posButtonSymbolLarge) | ||
| } | ||
| .foregroundColor(.posOnSurface) | ||
| }) | ||
| HStack(spacing: POSSpacing.none) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this should be 50/50 split...
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is definitely not 😂 , already addressed on #16038 |
||
| VStack(alignment: .leading, spacing: POSSpacing.none) { | ||
| List(selection: $selection) { | ||
|
Comment on lines
+17
to
+19
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider breaking these up into subviews... aim for something clearer at the top level like: You'll need to pass some bindings around but it'll be both easier to read/debug in future, and should perform better (not that there's a performance issue!) |
||
| Section { | ||
| ForEach([SidebarNavigation.store, SidebarNavigation.hardware], id: \.self) { item in | ||
| HStack { | ||
| Image(systemName: item.icon) | ||
| .font(.posBodyLargeRegular()) | ||
| VStack(alignment: .leading) { | ||
| Text(item.title) | ||
| .font(.posBodyLargeRegular()) | ||
| Text(item.subtitle) | ||
| .font(.posBodyMediumRegular()) | ||
| .foregroundStyle(.secondary) | ||
| } | ||
| } | ||
| .tag(item) | ||
| } | ||
| } | ||
| } | ||
| .safeAreaInset(edge: .bottom) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wasn't able to pin the We do not necessarily need a list here and perhaps creating just a reusable "card" would work fine for the 3 items. If we do, this can be iterated on a separate PR. Thoughts?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, if a list makes it harder, let's just make our own cards. Also, this might move the help button above an on-screen keyboard, which would be a bit strange. It's likely that we'll end up presenting one in settings eventually. |
||
| Button { | ||
| selection = .help | ||
| } label: { | ||
| HStack { | ||
| Image(systemName: SidebarNavigation.help.icon) | ||
| .font(.posBodyLargeRegular()) | ||
| .foregroundStyle(selection == .help ? .white : .primary) | ||
| VStack(alignment: .leading) { | ||
| Text(SidebarNavigation.help.title) | ||
| .font(.posBodyLargeRegular()) | ||
| .foregroundStyle(selection == .help ? .white : .primary) | ||
| Text(SidebarNavigation.help.subtitle) | ||
| .font(.posBodyMediumRegular()) | ||
| .foregroundStyle(selection == .help ? .secondary : .secondary) | ||
| } | ||
| Spacer() | ||
| } | ||
| .padding(.vertical, POSPadding.small) | ||
| .padding(.horizontal, POSPadding.medium) | ||
| .contentShape(Rectangle()) | ||
| } | ||
| .buttonStyle(.plain) | ||
| .background( | ||
| RoundedRectangle(cornerRadius: POSCornerRadiusStyle.large.value, style: .continuous) | ||
| .fill(selection == .help ? Color.accentColor : Color.clear) | ||
| ) | ||
|
Comment on lines
+60
to
+63
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The selected version looks pretty odd, being purple while the other selected items go grey. Having rounded corners doesn't work well with the padding we have here either – which is closer to the screen edges because it's outside the list, but if we keep it this way then square corners would make more sense. I know UI's not final so no need to fix here, but thought I'd mention it. |
||
| } | ||
| } | ||
| Group { | ||
| switch selection { | ||
| case .store: | ||
| PointOfSaleSettingsStoreDetailView() | ||
| case .hardware: | ||
| PointOfSaleSettingsHardwareDetailView() | ||
| case .help: | ||
| PointOfSaleSettingsHelpDetailView() | ||
| default: | ||
| EmptyView() | ||
| } | ||
| } | ||
| .frame(maxWidth: .infinity, maxHeight: .infinity) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| extension PointOfSaleSettingsView { | ||
| enum HardwareDestination: Identifiable, CaseIterable { | ||
| case cardReaders | ||
| case scanners | ||
|
|
||
| var id: Self { self } | ||
|
|
||
| var title: String { | ||
| switch self { | ||
| case .cardReaders: | ||
| return Localization.hardwareNavigationCardReaderTitle | ||
| case .scanners: | ||
| return Localization.hardwareNavigationBarcodeTitle | ||
| } | ||
| } | ||
|
|
||
| var subtitle: String { | ||
| switch self { | ||
| case .cardReaders: | ||
| return Localization.hardwareNavigationCardReaderSubtitle | ||
| case .scanners: | ||
| return Localization.hardwareNavigationBarcodeSubtitle | ||
| } | ||
| } | ||
|
|
||
| var icon: String { | ||
| switch self { | ||
| case .cardReaders: | ||
| return "creditcard" | ||
| case .scanners: | ||
| return "qrcode.viewfinder" | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
+83
to
+117
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It feels like overreach for this to be defined in the settings view. Surely this would be better in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True that, I'll address this one along #16038 , as |
||
|
|
||
| private extension PointOfSaleSettingsView { | ||
| enum SidebarNavigation: String, CaseIterable, Identifiable { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might be nicer to define a protocol for settings items instead, then each can be separately defined closer to their content? Not sure about this, just something to consider. |
||
| case store | ||
| case hardware | ||
| case help | ||
|
|
||
| var id: Self { self } | ||
|
|
||
| var title: String { | ||
| switch self { | ||
| case .store: return Localization.sidebarNavigationStoreTitle | ||
| case .hardware: return Localization.sidebarNavigationHardwareTitle | ||
| case .help: return Localization.sidebarNavigationHelpTitle | ||
| } | ||
| } | ||
|
|
||
| var subtitle: String { | ||
| switch self { | ||
| case .store: return Localization.sidebarNavigationStoreSubtitle | ||
| case .hardware: return Localization.sidebarNavigationHardwareSubtitle | ||
| case .help: return Localization.sidebarNavigationHelpSubtitle | ||
| } | ||
| } | ||
|
|
||
| var icon: String { | ||
| switch self { | ||
| case .store: return "bag" | ||
| case .hardware: return "wrench.and.screwdriver" | ||
| case .help: return "questionmark.circle" | ||
| } | ||
| } | ||
| } | ||
|
|
||
| enum Localization { | ||
| static let navigationTitle = NSLocalizedString( | ||
| "pointOfSaleSettingsView.navigationTitle", | ||
| value: "Settings", | ||
| comment: "Title of the Point of Sale settings view." | ||
| ) | ||
|
|
||
| static let sidebarNavigationStoreTitle = NSLocalizedString( | ||
| "pointOfSaleSettingsView.sidebarNavigationStoreTitle", | ||
| value: "Store", | ||
| comment: "Title of the Store section within Point of Sale settings." | ||
| ) | ||
|
|
||
| static let sidebarNavigationHardwareTitle = NSLocalizedString( | ||
| "pointOfSaleSettingsView.sidebarNavigationHardwareTitle", | ||
| value: "Hardware", | ||
| comment: "Title of the Hardware section within Point of Sale settings." | ||
| ) | ||
|
|
||
| static let sidebarNavigationHelpTitle = NSLocalizedString( | ||
| "pointOfSaleSettingsView.sidebarNavigationHelpTitle", | ||
| value: "Help", | ||
| comment: "Title of the Help section within Point of Sale settings." | ||
| ) | ||
|
|
||
| static let sidebarNavigationStoreSubtitle = NSLocalizedString( | ||
| "pointOfSaleSettingsView.sidebarNavigationStoreSubtitle", | ||
| value: "Store configuration and settings", | ||
| comment: "Description of the settings to be found within the Store section." | ||
| ) | ||
|
|
||
| static let sidebarNavigationHardwareSubtitle = NSLocalizedString( | ||
| "pointOfSaleSettingsView.sidebarNavigationHardwareSubtitle", | ||
| value: "Manage hardware connections", | ||
| comment: "Description of the settings to be found within the Hardware section." | ||
| ) | ||
|
|
||
| static let sidebarNavigationHelpSubtitle = NSLocalizedString( | ||
| "pointOfSaleSettingsView.sidebarNavigationHelpSubtitle", | ||
| value: "Get help and support", | ||
| comment: "Description of the Help section in Point of Sale settings." | ||
| ) | ||
|
|
||
| static let hardwareNavigationBarcodeTitle = NSLocalizedString( | ||
| "pointOfSaleSettingsView.hardwareNavigationBarcodeTitle", | ||
| value: "Barcode scanners", | ||
| comment: "Navigation title of Barcode scanner settings." | ||
| ) | ||
|
|
||
| static let hardwareNavigationCardReaderTitle = NSLocalizedString( | ||
| "pointOfSaleSettingsView.hardwareNavigationCardReaderTitle", | ||
| value: "Card readers", | ||
| comment: "Navigation title of Card reader settings." | ||
| ) | ||
|
|
||
| static let hardwareNavigationCardReaderSubtitle = NSLocalizedString( | ||
| "pointOfSaleSettingsView.hardwareNavigationCardReaderSubtitle", | ||
| value: "Manage card reader connections", | ||
| comment: "Description of Card reader settings for connections." | ||
| ) | ||
|
|
||
| static let hardwareNavigationBarcodeSubtitle = NSLocalizedString( | ||
| "pointOfSaleSettingsView.hardwareNavigationBarcodeSubtitle", | ||
| value: "Configure barcode scanner settings", | ||
| comment: "Description of Barcode scanner settings configuration." | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| #Preview { | ||
| PointOfSaleSettingsView() | ||
| } | ||
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 good to have this view handle back navigation for child views – the in-line navigation bar is a bit rough.
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.
Yes, in the first draft of designs that we've just been provided with, the Settings do not seem to have any header, so we may get rid of this bit entirely and leave navigation to the child/side view entirely. I'll iterate on this shortly 👍