Stats Review: Add Utilities (P6) - #24718
Conversation
Generated by 🚫 Danger |
|
| // Characters that require field escaping according to RFC 4180 | ||
| static let charactersRequiringEscape = CharacterSet(charactersIn: ",\"\r\n") | ||
|
|
||
| func generateCSV(from items: [any TopListItemProtocol], metric: SiteMetric) -> String { |
There was a problem hiding this comment.
Does the item need to be TopListItemProtocol? Can they just be CSVExportable?
If they do need to be both, you can declare the function as below to avoid casting:
func generateCSV<T: TopListItemProtocol + CSVExportable>(from items: [T], metric: SiteMetric) -> StringThere was a problem hiding this comment.
The app uses [any TopListItemProtocol] throughout (the alternative would've required to make a ton of types generic making them more complicated than needed), so we don't know the exact type at compile time when we invoke generate.
| @@ -0,0 +1,19 @@ | |||
| import SwiftUI | |||
|
|
|||
| struct CardModifier: ViewModifier { | |||
There was a problem hiding this comment.
Nitpick: This modifier does not seem to be necessary. I think we can move the function body to func cardStyle()?
There was a problem hiding this comment.
It's not strictly necessary, but it's an idiomatic approach if you have multiple adjustments to make. Typically, you would use a modifier when you have something more complex: need to access environment values, etc. It does produce a shorter type signature, which is easier to see during debugging and (might) have performance implications, so I think modifiers are generally the way to go for scenarios like this.
|
|
||
| extension View { | ||
| @ViewBuilder | ||
| func trackScrollOffset(isScrolling: Binding<Bool>) -> some View { |
There was a problem hiding this comment.
If I'm not mistaken, the ScrollOffsetModifier checks whether the scroll view has been scrolled, not whether it's currently scrolling? If that's the case, the name isScrolling might be a bit misleading?
There was a problem hiding this comment.
Yeah, I forgot to update it. Done.
| import Foundation | ||
|
|
||
| struct SelectedDataPoints { | ||
| let current: DataPoint? |
There was a problem hiding this comment.
Should this property be optional? None of the static functions below creates an instance with current = nil.
There was a problem hiding this comment.
They do as on a chart you can select an area (date) either the current point, or the previous point, or both, or with no points (SelectedDataPoints?).
| /// A completed formatted trend with the absolute change and the percentage change. | ||
| var formattedTrendShort2: String { | ||
| "\(formattedChange) \(iconSign) \(formattedPercentage)" | ||
| } |
There was a problem hiding this comment.
The two "short" strings are both longer than formattedTrend.
There was a problem hiding this comment.
Also, maybe we don't want to have two variables named foo and foo2?
There was a problem hiding this comment.
I made the following changes:
- Inline
formattedTrendShortinWeeklyTrendsViewas it's the only place where it's used - Rename
formattedTrendtoformattedTrendShort - Rename
formattedTrendShort2toformattedTrendShort
Not great that it uses two spaces there, but I'd keep this for now.
Commit: 3738445
3 participants
The PR adds small utilities and views like
CSVExporter,CardModifier,ScrollOffsetModifierfor compatibility with previous versions and more.One notable change is
TrendViewModelthat shows trends/change over time. See the unit tests to learn what it does and how.