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

feat: Tracks saving #271

Merged
merged 148 commits into from
Apr 30, 2024
Merged

feat: Tracks saving #271

merged 148 commits into from
Apr 30, 2024

Conversation

CDFN
Copy link
Collaborator

@CDFN CDFN commented Apr 22, 2024

This PR adds hooks for backend communication and implements tracks saving.

Closes #239

CDFN and others added 30 commits April 11, 2024 10:25
Copy link
Member

@achou11 achou11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My previous comment about the custom navigation header that's used in the save tracks screen seems to have been overlooked. that's probably the biggest blocker on my end right now.

Also had a UX question about something i ran into when testing this in the app.

Copy link
Member

@achou11 achou11 Apr 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CDFN @bohdanprog highlighting this in case this got lost in the first review

EDIT: Github does a terrible job of re-surfacing this, so adding the direct comment link

Copy link
Contributor

@ErikSin ErikSin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides my 2 comments, and andrew's comments, looks good to me.

I have noticed some architecture changes related to the location provider that I think we need to revert, but I will create an issue for that.

I realize that we are not persisting the tracks (after the track has been taken). I added a comment below

src/frontend/hooks/tracks/useTracking.ts Outdated Show resolved Hide resolved
navigation,
}) => {
const {formatMessage: t} = useIntl();
const [description, setDescription] = useState('');
Copy link
Contributor

@ErikSin ErikSin Apr 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to persist the description in case the user closes their phone. instead of using setDescription use updateTags('notes', newVal); and instead of description use const notes = usePersistedDraftObservation(store => store.value?.tags.notes)

@ErikSin
Copy link
Contributor

ErikSin commented Apr 30, 2024

If the user has stopped "tracking" but is in the middle of saving it (aka they are on SaveTrackScreen) and they close the app, we want to make sure it opens again on the saveTrackScreen, and that the tracks are persisted in state. In observations acheive this by saving observations in persisted state.

We should follow a similar pattern with tracks. Instead of saving locationHistory to plain zustand state we should save it to persisted state.

Also, when the app open we want to do a check to see if locationHistory exists in persisted state, and if it does, open the SaveTrackScreen.

^^When we do this changes the navigation behaviour. When the hit save, they will be navigated to the map. But the user will now be able to press the back button and go back to the saveTrackScreen. So we need to prevent this by resetting the nav stack on save.

@digidem digidem deleted a comment from expo bot Apr 30, 2024
@digidem digidem deleted a comment from expo bot Apr 30, 2024
@achou11
Copy link
Member

achou11 commented Apr 30, 2024

If the user has stopped "tracking" but is in the middle of saving it (aka they are on SaveTrackScreen) and they close the app, we want to make sure it opens again on the saveTrackScreen, and that the tracks are persisted in state. In observations acheive this by saving observations in persisted state.

We should follow a similar pattern with tracks. Instead of saving locationHistory to plain zustand state we should save it to persisted state.

Also, when the app open we want to do a check to see if locationHistory exists in persisted state, and if it does, open the SaveTrackScreen.

^^When we do this changes the navigation behaviour. When the hit save, they will be navigated to the map. But the user will now be able to press the back button and go back to the saveTrackScreen. So we need to prevent this by resetting the nav stack on save.

@ErikSin thoughts on this being a follow-up to this PR? The scope of changes required to get the above working might be large enough to warrant a separate PR to make it easier to review. Not sure if @CDFN @bohdanprog have already started making changes here towards that but if they haven't, would put it into consideration

@ErikSin
Copy link
Contributor

ErikSin commented Apr 30, 2024

@ErikSin thoughts on this being a follow-up to this PR? The scope of changes required to get the above working might be large enough to warrant a separate PR to make it easier to review. Not sure if @CDFN @bohdanprog have already started making changes here towards that but if they haven't, would put it into consideration

Yeah, that sounds good. I do think we should do this sooner, rather than later as it will effect the subsequent Pr, but I think separating it makes sense.

@achou11
Copy link
Member

achou11 commented Apr 30, 2024

Yeah, that sounds good. I do think we should do this sooner, rather than later as it will effect the subsequent Pr, but I think separating it makes sense.

yeah agreed - would it be helpful to create an issue that details the needed changes that Erik described @CDFN @bohdanprog ?

@CDFN
Copy link
Collaborator Author

CDFN commented Apr 30, 2024

I got some changes but it's fairly easy to move it to another branch. For issue, would be great to summarize it there if you could.

@achou11 achou11 mentioned this pull request Apr 30, 2024
3 tasks
@CDFN
Copy link
Collaborator Author

CDFN commented Apr 30, 2024

@achou11 does this mean we should merge this one and work on #306 right after? Or just merge #306 into feat/save-track and then merge this pr into main?

@achou11
Copy link
Member

achou11 commented Apr 30, 2024

@achou11 does this mean we should merge this one and work on #306 right after? Or just merge #306 into feat/save-track and then merge this pr into main?

The first option is what I would suggest, so you aren't doing too much branch managing :)

Copy link
Member

@achou11 achou11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this comment about the navigation header be addressed before merging?

Otherwise changes seem ready for merging :)

@bohdanprog
Copy link
Collaborator

bohdanprog commented Apr 30, 2024

Can this comment about the navigation header be addressed before merging?

Otherwise changes seem ready for merging :)

Hm...but I already changed Header in SaveTrackScreen :D

Here is a link :)
https://github.com/digidem/comapeo-mobile/pull/271/files#diff-26c6b74f7f8821ec00c3bbe4c3af65fc469d5d9aca1500e13eeec62733009ea9R95

@ErikSin
Copy link
Contributor

ErikSin commented Apr 30, 2024

Im going to merge this as I have a PR stacked on this, and I would like to create another PR based on this

@ErikSin ErikSin merged commit 0359735 into main Apr 30, 2024
3 checks passed
@ErikSin ErikSin deleted the feat/save-track branch April 30, 2024 19:45
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.

Saving or discarding tracks
5 participants