🍎 Adjust Permissions UI to Comply with iOS Guidelines #1182
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Unify setupLocChecks and setupFitnessChecks, which were split into different versions for ios and android but were mostly the same repeated code Use AlertManager to trigger alerts directly instead of passing error, errorVis, and setErrorVis through the hook and triggering them from PermissionsControls Split the 'description tag' logic for location settings & permissions into separate functions since they vary by platform and version In the checks object: Rename "statusState" to "status" Remove "statusColor" and "statusIcon" since they can be derived from "status". Handle the color and icon in PermissionItem.tsx. Add "isOptional" property Remove other unused code like haveSetText and setHaveSetText
Having com.unarin.cordova.beacon after cordova-plugin-em-datacollection turned out to be a problem because strings such as NSLocationAlwaysUsageDescription were being overwritten, so the location requests said things like "This app would like to scan for iBeacons even when in the background." I moved com.unarin.cordova.beacon just above all the "cordova-*" plugins because that is where it would go alphabetically anyway
I changed this to accept the unmodified check with newProps as a separate parameter. It turns out the old implementation where the check was mutated was actually a behavior being relied on; so I am reverting
Although the notifs perm is optional, we do want to make sure the user is at least prompted for it. We need to ensure that overallStatus stays false until notif perms have been requested. We keep track of 'wasRequested', overallStatus will only pass if all checks are status=true or they are optional and have already been requested (ie they were explictly denied) We can also have this reflected in the UI; use 'outline' icons to indicate a check that was not yet prompted and use 'filled in' icons to indicate an explicit choice. Moved this to a separate function iconAndColorForCheck for tidiness
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1182 +/- ##
==========================================
+ Coverage 29.12% 29.42% +0.30%
==========================================
Files 122 122
Lines 4961 4910 -51
Branches 1129 1080 -49
==========================================
Hits 1445 1445
+ Misses 3514 3465 -49
+ Partials 2 0 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
JGreenlee
changed the title
JGreenlee
changed the title
'Precise' location did not exist until ios 14 so there is no point in describing it in the ios 13-13.3 prompt
If notification permissions are denied, the description text will be updated telling the user they will not receive reminders or alerts. We do not need to also put this text in an alert (SnackBar)
shankari
approved these changes
Oct 7, 2024
There was a problem hiding this comment.
I am so glad that you were able to fix this
db19f35
I remember implementing this hackily to get it out of the door, and I am so happy that you were able to go in and clean it up. I do think we need to think more carefully about our handling of optional permissions, but this is already a big step up from what we had before.
At this point, you know this code better than I do, so this is just a high level sanity check.
www/i18n/en.json
Outdated
| } | ||
| }, | ||
| "overall-fitness-name-android": "Physical activity", | ||
| "overall-fitness-name-ios": "Motion and Fitness", | ||
| "overall-fitness-description": "The fitness sensors distinguish between walking, bicycling and motorized modes. We use this data in order to separate the parts of multi-modal travel such as transit. We also use it to as a cross-check potentially spurious trips - if the location sensor jumps across town but the fitness sensor is stationary, we can guess that the trip was invalid.", | ||
| "overall-fitness-description": "The fitness sensors distinguish between walking, bicycling and motorized modes. We use this data in order to separate the parts of multi-modal travel such as transit. We also use it to as a cross-check potentially spurious trips - if the location sensor jumps across town but the fitness sensor is stationary, we can guess that the trip was invalid.", | ||
| "fitnessperms": { | ||
| "name": "Fitness Permission", | ||
| "description": { | ||
| "android": "Please allow.", |
There was a problem hiding this comment.
nit, future fix: if we are going to list out the details, we should do so on android as well.
www/i18n/en.json
Outdated
| } | ||
| }, | ||
| "overall-notification-name": "Notifications", | ||
| "overall-notification-description": "We need to use notifications to inform you if the settings are incorrect. We also use hourly invisible push notifications to wake up the app and allow it to upload data and check app status. We also use notifications to remind you to label your trips.", | ||
| "overall-notification-description": "We use notifications to inform you if the settings are incorrect. We also use hourly invisible push notifications to wake up the app and allow it to upload data and check app status. We also use notifications to remind you to label your trips.", |
There was a problem hiding this comment.
we may be able to remove this "We also use hourly invisible push notifications to wake up the app and allow it to upload data and check app status." now that we know that we don't need the notification permission for it.
shankari
added a commit
to shankari/e-mission-data-collection
that referenced
this pull request
Oct 7, 2024
…hecker As part of e-mission/e-mission-docs#1094, we made the notification permission optional. Since it is not required for silent push notifications, notifications are nice-to-have. They are still helpful to ensure that people know if there are issues, and hopefully help them label their trips, but the app will still work if the permission is turned off. Since this is no longer required for normal operation, we don't need to check it in the background every hour, and nag the user to fix it if it is turned off. This is a super-easy change (two lines), so rolling this in while fixing e-mission/e-mission-docs#1094 instead of coming in as a future fix. Testing done: - Replaced the data collection plugin with the most recent version - Code compiles - Wasn't able to test in the emulator since with the permission turned off, without e-mission/e-mission-phone#1182 in place, we got a prompt to fix the notification, so we couldn't simulate a remote push Pushing this and testing all the changes together
- update overall-notification-description to not mention "hourly invisible push notifications" since they do not depend on the permission -adjust some Android strings to be more congruent with the iOS versions
|
@JGreenlee android build broke because of xml formatting |
|
Reproduced locally, fixing and updating the release... |
|
The code coverage action failure is also due to a failing test, but it looks like it is a timeout. So I am going ahead and merging this and pushing a release out on to staging now... |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The "notifications" permission is now optional and the UI represents this with different icons and the "warn" (yellow) color. If the user denies the notifications permission, they get a warning but can still proceed through onboarding.
While doing this, I significantly refactored and simplified
usePermissionStatus.tsbecause it had a lot of repeated and unnecessarily complex code.Also, the language used to describe the permissions has been made more descriptive, and reworded to remove the possibility of it being perceived as persuading or influencing users