Orders: Infinite Scroll Support #295
Conversation
jleandroperez
requested review from
bummytime and
mindgraffiti
and removed request for
bummytime
Generated by 🚫 Danger |
|
@jleandroperez, our first |
bummytime
left a comment
bummytime
left a comment
There was a problem hiding this comment.
@jleandroperez First off smoke testing — I tested the heck out of this for 30 minutes and could not get it to fail for me! Fast network, slow network, pull-to-refresh while syncing, dismissing the app while syncing, etc. It worked like a champ. ✅
❤️ed the unit tests! ✅
Code-wise I think this is great. I had a few general comments, but nothing that's a showstopper. I really like this new Coordinator...very well-thought out and straight-forward. ✅
from me!!
| static let defaultPageSize: Int = 75 | ||
| public extension OrdersRemote { | ||
| public enum Defaults { | ||
| public static let pageSize: Int = 75 |
There was a problem hiding this comment.
I'm assuming we will lower this to 25 based on Slack discussion?
|
|
||
| /// SyncingCoordinator: Encapsulates all of the "Last Refreshed / Should Refresh" Paging Logic. | ||
| /// | ||
| class SyncingCoordinator { |
There was a problem hiding this comment.
FWIW, I think this name is just fine. 🤷♂️
| } | ||
|
|
||
|
|
||
| // MARK: - Analyzer Methods: For unit testing purposes, marking them as `Internal` |
There was a problem hiding this comment.
Nice! Unit test hooks FTW!
| } | ||
|
|
||
| enum Syncing { | ||
| static let pageSize = 40 |
There was a problem hiding this comment.
Just like the comment above ☝️ should this one be 25? 😄
|
@bummytime thanks a lot for the review!!!
Woooooo thank you!!! |
Details:
This PR implements a new tool, SyncingCoordinator, which is in charge of:
It comes with batteries and unit tests.
Important Note:
The first page is considered a "Special Case", not dealt with by the Sync Coordinator. The main drive behind this decision is: we want to always sync the first page, whenever the user opens the Orders tab, no matter what.
This new component (SyncingCoordinator) handles all of the Paging-Y related logic.
Testing:
@bummytime @mindgraffiti please kill and try to break this code!!!. Any suggestions will be more than welcome, including a better name for the "SyncingCoordinator".