Skip to content
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

Merged
merged 2 commits into from
Oct 21, 2024

Conversation

kondayutarou
Copy link
Contributor

@kondayutarou kondayutarou commented Oct 20, 2024

What it Does

How I Tested

  • Run the application
  • Add a vehicle from the setting
  • Add a reading on the Odometer page

Notes

  • None

Screenshot

Copy link
Owner

@mikaelacaron mikaelacaron left a 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)
Copy link
Owner

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 {
Copy link
Owner

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))
Copy link
Owner

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

Comment on lines 12 to 18
private let reading: OdometerReading
private let vehicleName: String?

init(odometerReading: OdometerReading, vehicleName: String?) {
self.reading = odometerReading
self.vehicleName = vehicleName
}
Copy link
Owner

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

@mikaelacaron mikaelacaron merged commit 5e5ed93 into mikaelacaron:dev Oct 21, 2024
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IMPROVE - How the mileage is displayed on the OdometerView
2 participants