-
Notifications
You must be signed in to change notification settings - Fork 1
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/tracking impl #257
Feat/tracking impl #257
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.
How difficult would it be to revert significant styling changes that are beyond the scope of this feature? for example, reverting the UI changes made to:
- The GPS pill
- The big action buttons
- tab navigation container
main reason i ask is mostly related to stuff we've talked about elsewhere in terms of limiting the focus of the changes you introduce. would also help reduce the size of PRs in some cases :)
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.
What you think about wrapping Home (rendered via HomeTabs component) screen with context we created?
Yeah that makes sense to me 👍
What would be benefits of using useSyncExternalStore vs zustand store?
i don't really have strong thoughts on one vs the other. I'm just more familiar with how to deal with subscriptions (i.e. managing event listeners) with useSyncExternalStore than zustand. if it's straightforward to implement with zustand, i'm all for it!
Also subscribing with different parameters will be the trickiest part. Easiest way would be having a store storing few locations with their own subscriptions and different update periods. There could be 3 pre-configured levels of refresh period (slow, medium, fast). You can just subscribe to whatever update frequency you want via selector
yeah definitely the trickiest part. depending on if i'm reading the suggestion here correctly, it could be interesting!
- one way i'm reading it is that you're suggesting having 3 location subscriptions set up (i.e.
addEventListener()
3 times). i'd be worried about due to the potential cost of having multiple active location subscriptions (not sure how the native side is implemented). - the other way is that you're suggesting 1 location subscription, but having selectors (e.g. zustand selector) that do the hard work of determining the update frequency (e.g. based on distance difference)
…e into feat/tracking-impl
|
Yeah apologies for the confusion. I originally asked the questions because of uncertainties we had internally around the more global design changes we wanted to introduce. Will clarify in our chat on Slack with what we've agreed upon internally, but the short sumary is that you can ignore my question about reverting UI changes and instead keep what you have implemented :)! |
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.
Adding a note about implementing a feature flag setup so that the tracks feature(s) can be hidden behind a flag so as to not be exposed by default. In this PR, the flag should determine whether the tracks tab is rendered
src/frontend/Navigation/ScreenGroups/TabBar/TrackingTabBarIcon.tsx
Outdated
Show resolved
Hide resolved
src/frontend/Navigation/ScreenGroups/TabBar/CameraTabBarIcon.tsx
Outdated
Show resolved
Hide resolved
about that, I found that
|
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.
Changes around the feature flag look good!
Think I mentioned this regression before, but think it should be fixed before merging. Reproduction steps:
- Navigate to camera tab
- Press big button
- Press button in top-left to discard observation (i.e. leave observation creation flow)
After (3), you end up in the map tab, but you should end up in the camera tab.
According to my response here: |
Ah sorry, missed that! This is a regression compared to the old app, but as you said, this is not something that was introduced with your changes. Thanks for pointing that out! will create a separate issue that someone else can tackle separately :) |
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 initial work! hoping the next pieces will involve less back-and-forth 😄
Here are three tasks in a single PR; unfortunately, they are linked:
Next time, we plan to approach them partially, hoping for a clearer outcome.
Resolves #236
Resolves #237
Resolves #238