-
Notifications
You must be signed in to change notification settings - Fork 108
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
Notifications: New Order Details #431
Conversation
This reverts commit f3b54fb.
SwiftLint found issuesWarnings
Generated by 🚫 Danger |
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.
✅ success flow
✅ failure flow
✅ code
(reminder) Dangerbot complaint
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.
So I looked hard for something to complain about here — just couldn't find anything. :-)
I smoke tested this pretty well:
✅ All of the order details-specific functionality worked as expected (add a note, complete order, etc)
✅ Overall screen looks great on a variety of devices (portrait and land)
✅ Code is square
✅ Unit tests are good
// MARK: - OrderLoaderViewController: Loads asynchronously an Order (given it's OrderID + SiteID). | ||
// On Success the OrderDetailsViewController will be rendered "in place". | ||
// | ||
class OrderLoaderViewController: UIViewController { |
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 like the entire concept of this VC subclass — very simple and straight-forward way to spin up the order details VC. A+
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.
Patching OrderDetailsViewController
would have been a nightmare! Thank you Matt!!
Thank you both!! |
Details:
In this PR we're implementing a new "Loader", to be called OrderLoaderViewController.
This tool allows us to display a spinner, in place, async load an Order (given its orderID + siteID), and render the OrderDetails, right there, in place.
cc @bummytime @mindgraffiti
Thanks in advance!!
Testing: Successful Flow
Verify that a ViewController shows up immediately. A spinner should kick off, and the Order Details should appear shortly.
Testing: Failure Flow
Verity that a spinner shows up immediately. Shortly after, the "Failure Overlay" should appear onscreen, with a retry button.