-
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
[POS Settings] Add two-panel settings view #16021
Conversation
It doesn’t seem to be straight-forward to push one of the elements of a List down to the bottom, while keeping the rest at the top, so “Help” has to be outside the List.
We also remove the navigation stack from the settings view as a parent, otherwise is part of the nav stack, and presenting any other destination would cover it full-screen. We only make the details right-side part of the nav stack instead.
Dismissal does not show anymore in the parent settings view since we don’t us a navigation stack, and we cannot use a navigation stack otherwise the destinations will be presented full-screen, so we move dismissal to each subview.
Generated by 🚫 Danger |
| } | ||
| } | ||
| } | ||
| .safeAreaInset(edge: .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 wasn't able to pin the Help section to the bottom of the view while this is part of a List. This approach worked, but I'm not fully convinced is the best way to move forward.
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?
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.
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.
| .foregroundStyle(.secondary) | ||
| } | ||
| .padding() | ||
| .toolbar { |
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.
Here's a NavigationStack gotcha: I had to add the .toolbar for the dismiss action to all destination views, rather than once to the parent Settings view, if we do, then these destinations will be presented full-screen as part of the stack, rather than encapsulated to the right-side only.
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.
An alternative that seems to work is to dismiss the no longer needed since using a PointOfSaleSettingsView through a viewmodifier attached to the parent view instead, so we could remove the .toolbar approach from the subviews. For testing I used .posModalCloseButton(action: { dismiss() }) since already exists, and seemed to work fine.POSPageHeaderView: 86fe351
|
|
| Text(Localization.navigationTitle) | ||
| .font(.posHeadingRegular) | ||
| .padding(.horizontal, POSPadding.medium) | ||
| .padding(.top, POSPadding.medium) |
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.
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.
Updated on 86fe351
joshheald
left a comment
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.
Some suggestions inline.
Works as described, fine to merge behind the feature flag.
Did you try using split views? My concern with the HStack approach is that it can be harder to manage at smaller screen sizes when we want to collapse it. With the coming iOS 26 having resizable windows, it might be best to take that pain up front and get it working with a split view instead of an HStack...
| POSPageHeaderView( | ||
| title: Localization.navigationTitle, | ||
| trailingContent: { | ||
| Button(action: { dismiss() }) { | ||
| Text(Image(systemName: "xmark")) | ||
| .font(.posButtonSymbolLarge) | ||
| } | ||
| .foregroundColor(.posOnSurface) | ||
| }) |
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 👍
| } | ||
| } | ||
| } | ||
| .safeAreaInset(edge: .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.
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.
| .background( | ||
| RoundedRectangle(cornerRadius: POSCornerRadiusStyle.large.value, style: .continuous) | ||
| .fill(selection == .help ? Color.accentColor : Color.clear) | ||
| ) |
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.
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.
| } | ||
|
|
||
| private extension PointOfSaleSettingsView { | ||
| enum SidebarNavigation: String, CaseIterable, Identifiable { |
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.
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.
| 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" | ||
| } | ||
| } | ||
| } | ||
| } |
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 feels like overreach for this to be defined in the settings view. Surely this would be better in PointOfSaleSettingsHardwareDetailView?
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.
True that, I'll address this one along #16038 , as PointOfSaleSettingsView.HardwareDestination changes 👍
| HStack(spacing: POSSpacing.none) { | ||
| VStack(alignment: .leading, spacing: POSSpacing.none) { | ||
| List(selection: $selection) { |
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.
Consider breaking these up into subviews... aim for something clearer at the top level like:
var body: some View {
POSPageHeaderView(
...
)
HStack {
PointOfSaleSettingsList()
PointOfSaleSettingsDetail()
}
}
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!)
| } | ||
| .foregroundColor(.posOnSurface) | ||
| }) | ||
| HStack(spacing: POSSpacing.none) { |
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 don't think this should be 50/50 split...
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 definitely not 😂 , already addressed on #16038




Closes WOOMOB-125
Description
This PR adds a functional two-side navigation for POS settings. UI is not final, just temporary to build the settings scaffolding and navigation. UI details will be handled separately.
... > SettingsHardwarehas been added with barcode scanner, and card readers sub-navigation. The other views will present just a placeholder.Testing information
SwitchpointOfSaleSettingsi1totrue..., tap onSettingsHardware, observe there are two more items to navigate to: Card readers and barcode scannerScreenshots
Screen.Recording.2025-08-22.at.16.05.17.mov
RELEASE-NOTES.txtif necessary.