Dashboard Mark 6: Adds charts to dashboard #247
Conversation
… feature/177-dashboard-mark6 * 'develop' of github.com:woocommerce/woocommerce-ios: Code review changes: `public` declaration not needed for cell variables, `guard` instead of `if let` Updating Authenticator to Mark 1.0.6 Missed a whitespace Support empty addresses `pod update` Code review changes: make it swift-y Order List: Improve custom payment labels AppDelegate: Nukes unneeded self OrderStoreTests: Fixing Typo SettingsViewController: Simplifies downcasting MockupNetwork: Nukes extra dot AlamofireNetwork: Nukes extra space Order: Simplifies parsing # Conflicts: # Podfile.lock
mindgraffiti
left a comment
mindgraffiti
left a comment
There was a problem hiding this comment.
ui testing ✅
code review ✅
|
|
||
| /// Returns the sum of total sales this stats period. This value is typically used in the dashboard for revenue reporting. | ||
| /// | ||
| /// *Note:* The value returned here is an aggrigation of all the `OrderStatsItem.totalSales` values and |
There was a problem hiding this comment.
nitpicky - aggregation instead of aggrigation
jleandroperez
left a comment
jleandroperez
left a comment
There was a problem hiding this comment.
THIS LOOKS AWESOME!!!!!
Few nipticky comments here and there (feel free to disregard if the rounding is not really needed!!!)
| offset.y = chart.bounds.size.height - origin.y - height - padding | ||
| } | ||
|
|
||
| return offset |
There was a problem hiding this comment.
Suggestion: Rounding just in case the offset is not integral?
| rect.origin.x -= size.width / 2.0 | ||
| rect.origin.y -= size.height | ||
|
|
||
| context.saveGState() |
There was a problem hiding this comment.
Suggestion: CGRect.integral just in case the positions are not integers?
There was a problem hiding this comment.
Same suggestion for the whole method!
| extension PeriodDataViewController { | ||
| func clearAllFields() { | ||
| if barChartView != nil { | ||
| barChartView.clear() |
There was a problem hiding this comment.
Should probably be the same as barChartView?.clear() ?
| return | ||
| } | ||
|
|
||
| let marker: ChartMarker = ChartMarker(color: StyleManager.wooSecondary, |
There was a problem hiding this comment.
No need to specify the type (: ChartMaker)
|
|
||
| if size.height == 0.0 && image != nil { | ||
| size.height = image!.size.height | ||
| } |
There was a problem hiding this comment.
Suggestion: if let for the image, instead of the != nil and ! ?
|
Thanks for the reviews @mindgraffiti and @jleandroperez ! |
| } | ||
|
|
||
|
|
||
| // MARK: - Constants! |
There was a problem hiding this comment.
| MARK comment should be in valid format. e.g. ‘// MARK: …’ or ‘// MARK: - …’ |
designsimply
added
feature: stats
AliSoftware
added
feature: stats
Title has it — this PR is the continuation of work for #177. It uses the Charts library found here... a Swift version of the same lib that Woo Android uses. The chart itself has been styled in a similar fashion to Woo Android as well.
Chart Markers
For grins, I added interactive chart markers to the iOS version (which doubles as accessibility text for each bar item). Let me know your thoughts:
Testing
Log into a variety of stores and see if all 4 charts (days, weeks, months, years) look "ok" (as compared to similar data you would see on the web dashboard).
@mindgraffiti and @jleandroperez would you mind taking a peek?