search

heyjaywilson / peakfit

Strength exercise iOS app written in Swift
Apache License 2.0
14 stars 10 forks source link

allow a user to add an exercise to a list that already exists #15

Open heyjaywilson opened 3 weeks ago

heyjaywilson commented 3 weeks ago

In a list that already exists, a user needs to be able to add an exercise.

Use the symbol plus.circle in the trailing spot of the navigation bar.

Note: All writes to swift data should be done on a background actor. There is a Service already setup for ExerciseList

samuelsainz commented 2 weeks ago

@heyjaywilson hey Jay, I'd like to tackle this one, thanks in advance!

heyjaywilson commented 2 weeks ago

@samuelsainz go for it! Please note the addition about using the ExerciseList.Service to write to SwiftData

samuelsainz commented 4 days ago

@heyjaywilson hey Jay, I have a couple of questions regarding this implementation.

  1. What's the reason why you're using ExerciseList.Service to add or remove exercises from a list? I wanted to understand better why there is a custom implementation to addOrRemoveExercise. I think we could pass the exerciseList as a parameter to the SelectExerciseView and have it there as a @Bindable property. This way, you can leverage SwiftData to keep the model updated in the context, we wouldn't need to Query the exerciseList in the SelectExerciseView and we wouldn't need to query the exercise in the Service.

  2. Have you considered using NavigationPath to manage the navigation stack?

I figured I would ask you about these two things before making any decisions on how to implement this task. Thanks in advance!

heyjaywilson commented 3 days ago

What's the reason why you're using ExerciseList.Service to add or remove exercises from a list?

I like to keep writes on a background thread and then when it's saved the main thread will update. This is how I have handled a lot of core data projects and it's just clean especially if at some point there's a server that needs to be read from and update the data to.

My understanding is by doing the @Query the way it's currently written is that it is more reliably going to update itself from the background thread or any other update that could happen.

heyjaywilson commented 3 days ago

Have you considered using NavigationPath to manage the navigation stack?

It wasn't necessary at the time. I was doing details, so being able to just pass a variable to the view worked at the time. I'm not opposed to using a NavigationPath to handle the NavigationStack.

samuelsainz commented 2 days ago

Hi Jay, thanks a lot for the explanation, that makes sense!

I have implemented the feature in #67 with the minimal changes required, following the current approach.

Regarding my questions before, I would like to propose a couple of improvements:

  1. I'd recommend changing the SelectExerciseView to hold an ExerciseList instance as a @Bindable property. This would greatly simplify the code by eliminating the need to query the ExerciseList again, as the view is always opened in the context of an existing ExerciseList. By directly modifying exerciseList.exercises through @Bindable, we ensure that any changes are immediately reflected in the model and maintained in memory across the entire container.

  2. I saw that when you are in the ExerciseListDetailView, if you delete an exercise using the swipe gesture, the exercise is deleted not only for the list but for the entire app. I would expect that deleting the exercise there would just delete from the list

Please, let me know your thoughts—I'd be more than happy to help with anything. Thanks in advance!