-
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: Added NotificationStore and friends #396
Conversation
… issue/19-notifications-yosemite * 'develop' of github.com:woocommerce/woocommerce-ios: (37 commits) Better named variables AddANoteViewController: Wiring SwitchTableViewCell PrivacySettingsViewController: Wiring SwitchTableViewCell SwitchTableViewCell: Standard Subtitle UITableViewCell + Switch Write a unit test for a user opt-out scenario Missed a typo Clear tracks user data when a user opts out Fix broken pragma mark Give Privacy Settings table and Order Details table some empty space where footer would go Make changes for Crashlytics - turn off crash reporting if Collect info setting is turned off. Change method names from `optedOut` to `optedIn` Move URL constants to WooConstants Objective-C leaking into my brain. Removed parens around `if bool` Missed user defaults changes - default to user has not opted in to tracking Default to user has not opted in to analytics tracking. Remove optional for analytics provider (because it prevents unit testing) Add unit test for clearAllEvents() Touch up Tracks reporting for Settings Add logging Set up the interface to use the opt-out feature ...
… issue/19-notifications-yosemite * 'develop' of github.com:woocommerce/woocommerce-ios: Add env.example Add PRs list tool Updates Project NotificationsViewController: Wiring tabBar Initialization MainTabBarController: Removes unneeded setup Moving Notifications to its own Storyboard Implements Array+Notes
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.
Few nipticky comments sir!! Feel free to merge once ready!!
Amazing work!!!!
head = 0 | ||
} | ||
|
||
return element |
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.
Q: unless i'm missing something HUGE (which i probably am), wouldn't something like this work as well?
func dequeue() -> T? {
return array.removeFirst()
}
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.
@jleandroperez Yeah it definitely would. This code is an optimized version of the Queue data struct which has better performance.
Thanks for the review @jleandroperez !! |
This PR:
NotificationStore
andNotificationAction
StorageManagerType
(needed when saving/updating notifications in storage)MockupNetwork
such that a FIFO response queue can used — useful for testing the same endpoint where multiple responses are needed sequentially (e.g. notifications)Re: #19
Testing
@jleandroperez finally ready for a review now!