-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Log location coordinates in the background #2772
Conversation
… of simple boolean
…contain not only times but locations as well
…cting location coordinates
…om of the method which finishes loading forms
Agreed, 'Location Tracking' is better. I think right-aligned may be the norm for mobile. @yanokwa what you linked is for desktop/web UIs I think. See the screenshot I posted higher up... |
I saw the screenshot. Do you have a source so we can evaluate the context better? |
No, sorry. I forgot where I found that :( |
@yanokwa I was just about to say "Yeah, that sounds perfect"—and then I went looking for examples and found this in Chrome on Android: If the checkbox stays on the right, there needs to be a decent amount of padding between the text and checkbox. It's a shame to have this not match the snackbar, though. Are we open to putting it on the right in the snackbar? |
Wait what? There is a checkbox in the snackbar too? That seems odd to me... There shouldn't be UI controls in an ephemeral element like that. Will cause user anxiety—how to I get back to that?? |
Sorry, that was what I thought @yanokwa meant when he said "(to match the snack bar)". Rereading his comment, I'm seeing now that maybe he meant that the text "Location Tracking" should match the snack bar, rather than that the checkbox should match the snack bar. |
😅 |
Y'all are confusing me! So to confirm, the two changes we've agreed to make to the PR's UI.
|
Fixed.
Everything is fine here. |
@grzesiek2010 I noticed only that menu icon is not visible in snackbar on Android 4.2 and 4.1 Verified on Android: 8.1, 7.0, 6.0, 5.1, 4.4, 4.2, 4.1 Verified cases:
|
@opendatakit-bot unlabel "needs testing" |
So it's because some font's don't support that char. |
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.
@grzesiek2010 Please fix the merge conflicts and let's get this merged!
As to the glitch with Android 4, try replacing the ⋮
with ⋮
and see if that works better. If not, let's file a quick issue to improve it later.
I tried yesterday and no avail. A new issue is added #2813 |
Closes #2773
Closes #375
What has been done to verify that this works as intended?
I wrote some tests and performed manual testing but it's not enough of course and the pr should be tested carefully by our QAs.
Why is this the best possible solution? Were any other approaches considered?
I tried to follow the approach agreed in getodk/roadmap#28. Overall I was able to do everything as we discussed there apart from the section about
Data privacy issues
which I implemented in a bit different way.How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
The pr brings a lot of changes in old time logging functionality which now allows logging location coordinates as well, so the biggest risk is related to that option.
The code that manages collecting events is in FormEntryActivity so there I also had to implement some changes but It doesn't need any specific regression testing.
Do we need any specific form for testing your changes? If so, please attach one.
AuditTest.xml.txt
Does this change require updates to documentation? If so, please file an issue here and include the link below.
getodk/docs#934
Before submitting this PR, please make sure you have:
./gradlew checkAll
and confirmed all checks still pass OR confirm CircleCI build passes and run./gradlew connectedDebugAndroidTest
locally.