-
-
Notifications
You must be signed in to change notification settings - Fork 149
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
Reformat odometer view #346
Reformat odometer view #346
Conversation
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.
Great work!! I made several comments about small changes, mostly of things that I prefer for my coding style, that I've changed in your PR
Thanks for contributing!
|
||
var body: some View { | ||
VStack(alignment: .leading, spacing: 8) { | ||
Text("\(vehicleName ?? "No Name")").font(.title) |
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.
Let's use .title3
, because just .title
looks pretty big
also I personally like all modifiers to be on new lines, rather than on the same line so it'd look like:
Text("\(vehicleName ?? "No Name")")
.font(.title3)
|
||
import SwiftUI | ||
|
||
struct ReadingView: View { |
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'm going to rename this OdometerRowView
, as opposed to ReadingView
, to make it more clear
I fixed this
await viewModel.deleteReading(reading) | ||
let vehicleName = viewModel.vehicles.first { $0.id == reading.vehicleID }?.name | ||
ReadingView(odometerReading: reading, vehicleName: vehicleName) | ||
.listRowInsets(.init(top: 16, leading: 24, bottom: 16, trailing: 24)) |
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 think it's fine without these insets, I don't think it makes a dramatic difference
I fixed this
private let reading: OdometerReading | ||
private let vehicleName: String? | ||
|
||
init(odometerReading: OdometerReading, vehicleName: String?) { | ||
self.reading = odometerReading | ||
self.vehicleName = vehicleName | ||
} |
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.
these variables don't need to be private, and by making them not private, we don't need to manually create an initializer
I fixed this
What it Does
OdometerView
#336How I Tested
Notes
Screenshot