-
Notifications
You must be signed in to change notification settings - Fork 121
[Woo POS][Barcodes] Scanner set up flow structure #15883
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][Barcodes] Scanner set up flow structure #15883
Conversation
Generated by 🚫 Danger |
|
|
staskus
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.
Nice! 👍 I've done a few similar dynamic flows in other apps, I think it's done with the right balance:
- Keeping the concept of a scan selection static
- Adding title and button configurations, keeping the default option
- Allowing to define content for each step
I thought the best way to review it to try the ergonomics and defining a custom flow.
I'm not approving it yet, I see the the navigation within the flow steps is not working, I don't know if it's supposed to be in the scope or not, although the code is written in the way that I think it should but the currentContent is not updating when calling .nextStep()
WooCommerce/Classes/POS/Presentation/Barcode Scanner Setup/PointOfSaleBarcodeScannerSetup.swift
Show resolved
Hide resolved
| title: .constant(AttributedString(currentTitle))) | ||
|
|
||
| VStack { | ||
| currentContent |
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.
Maybe wrapping the content within the TabView will help with transition animations, although it could be tricky, especially if we don't know how many steps we have until the initial selection is made. Nevertheless, I would consider something like this here.
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... that's definitely worth trying, though I do want to keep it open to branching flows in future – e.g. additional steps after a failure for troubleshooting, going back to an arbitrary point in the flow.
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.
though I do want to keep it open to branching flows in future – e.g. additional steps after a failure for troubleshooting, going back to an arbitrary point in the flow.
TabView seems to be only partially suitable for this dynamic approach. Some online answers suggest that it's needed to change .id(..) of TabView whenever the content changes. I haven't tested it, though.
This is where it becomes interesting with the dynamic approach. Question whether we want to define within the steps of the flow more like a tree, to see possible paths if failures happen.
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.
Yeah, potentially. Let's avoid complicating it too much for now though.
The transitions in my early experiments could work pretty well like this, so we'll see how we get on 😊
Aside: controls for flows like this really feel like something a mobile platform should provide, given how common they are.
...erce/Classes/POS/Presentation/Barcode Scanner Setup/PointOfSaleBarcodeScannerSetupFlow.swift
Show resolved
Hide resolved
WooCommerce/Classes/POS/Presentation/Barcode Scanner Setup/PointOfSaleBarcodeScannerSetup.swift
Show resolved
Hide resolved
WooCommerce/Classes/POS/Presentation/Barcode Scanner Setup/PointOfSaleBarcodeScannerSetup.swift
Show resolved
Hide resolved
...erce/Classes/POS/Presentation/Barcode Scanner Setup/PointOfSaleBarcodeScannerSetupFlow.swift
Show resolved
Hide resolved
...ce/Classes/POS/Presentation/Barcode Scanner Setup/PointOfSaleBarcodeScannerSetupModels.swift
Show resolved
Hide resolved
| switch scannerType { | ||
| case .socketS720: | ||
| return [ | ||
| createWelcomeStep(title: "Socket S720 Setup") |
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.
This doesn't work fully, at least not yet. If I define more steps, the UI is not updated.
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.
Interesting. I'll have a play, thanks.
I would expect it to work, but I pulled multi-step flows out a while ago while focusing on the structure and getting the PR to a managable size. My guess is that there's some failing of observation going on.
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.
Wow, a simple fix indeed! Thanks. 👍
| ) | ||
| } | ||
|
|
||
| private var steps: [PointOfSaleBarcodeScannerSetupStep] { |
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.
This is nice, it will be handy to have all the steps in one place 👍
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.
Yeah. If we need to in future, we can pass them in and define them separately. I'm wary of generalising this too much right now, but also want to keep it open to that; we may have other flows in POS in future, e.g. printer set up, or even configuring a customer's choices for a subscription/booking cart item.
staskus
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.
🚢

Closes WOOMOB-695
Merge after #15882
Description
This PR adds some basic structure that we'll use to build the scanner set up flows. Sorry it's pretty big – I ended up having to move a lot of stuff around.
I'm doing this with a view to correctly transitioning between the views, keeping buttons and header more or less in the same place but having bigger transitions for the content. Not animated yet, this PR focuses on removing the use of NavigationStack.
We will define different steps for each scanner's flow, and then move along one flow at a time, handled by the
PointOfSaleBarcodeScannerSetupFlowManagerworking with multiple flows.Steps to reproduce
...and openBarcode Scanningto see the scanner selection screenDoneonly.Screenshots
scanner.flow.structure.mp4
RELEASE-NOTES.txtif necessary.