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

Added a more thorough Usage example, and make skipPermissionRequests optional #204

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

colinta
Copy link

@colinta colinta commented Oct 15, 2022

Overview

When I first looked at the repo, it wasn't clear that the permissions would be handled so easily. The example app has too many features, and so gives the impression that all that work is required to get it to work. The API is great, show it off!

I also changed some types for the configuration. The docs say that skipPermissionRequests defaults to false, but actually it's a required option according to the typescript definition.

Test Plan

I'm using this fork/branch in my own project - I tested the changes to setRNConfiguration and types on ios and android.

On Android, I tested the changes by logging the value here:

    protected static Configuration fromReactMap(ReadableMap map) {
      String locationProvider =
              map.hasKey("locationProvider") ? map.getString("locationProvider") : "auto";
      boolean skipPermissionRequests =
              map.hasKey("skipPermissionRequests") ? map.getBoolean("skipPermissionRequests") : false;
      System.out.println("=============== GeolocationModule.java at line 188 ===============");
      System.out.println("locationProvider:" + locationProvider);
      System.out.println("skipPermissionRequests:" + skipPermissionRequests);
      return new Configuration(locationProvider, skipPermissionRequests);
    }

And similarly on iOS

  NSDictionary<NSString *, id> *options = [RCTConvert NSDictionary:json];
  NSLog(@"options: %@", options);

and changed some types for the configuration
@@ -44,13 +44,13 @@ let updatesEnabled = false;
*/
export function setRNConfiguration(config: GeolocationConfiguration) {
RNCGeolocation.setConfiguration({
...config,
skipPermissionRequests: config.skipPermissionRequests,
Copy link
Author

Choose a reason for hiding this comment

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

skip... is now optional in config, so needs to be explicit here to remain consistent.

authorizationLevel:
config?.authorizationLevel === 'auto'
config.authorizationLevel === 'auto'
Copy link
Author

Choose a reason for hiding this comment

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

config is not undefined, only the property, but this check is safe for config.authorizationLevel === undefined.

@colinta
Copy link
Author

colinta commented Nov 1, 2022

@michalchudziak I can close if this PR is unnecessary (I hate to leave PRs open indefinitely). I could even break up the README change, if that seems more valuable than the type changes.

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.

None yet

1 participant