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

Improve request method documentation #726

Merged
merged 3 commits into from
Oct 21, 2022
Merged

Improve request method documentation #726

merged 3 commits into from
Oct 21, 2022

Conversation

fabiendem
Copy link
Contributor

Summary

The request method takes an optional rationale parameter.

This parameter is read on Android only:

async function request(permission: Permission, rationale?: Rationale): Promise<PermissionStatus> {

It is not read on iOS:

async function request(permission: Permission): Promise<PermissionStatus> {

Neither Windows:

function request(permission: Permission): Promise<PermissionStatus> {

This PR tweaks the documentation to make it clear that the Rationale is only useful on Android.

Happy to tweak the wording if needed.

Test Plan

No test plan, see updated README.

What's required for testing (prerequisites)?

What are the steps to test it (after prerequisites)?

Compatibility

OS Implemented
iOS
Android

Checklist

  • [] I have tested this on a device and a simulator
  • I added the documentation in README.md
  • [] I added a sample use of the API in the example project (example/App.tsx)

@zoontek
Copy link
Owner

zoontek commented Oct 21, 2022

@fabiendem Hi 👋 Thanks for the contribution!

I think having:

Note that the rationale parameter is only available and used on Android.

Will be more clear. No need to mention iOS here since rationales != prompt texts. What do you think?
PS: Don't forget to run prettier on the markdown or to remove the trailing spaces 🙂

@fabiendem
Copy link
Contributor Author

Hi @zoontek !
No probs, I have tweaked the wording there.
Format shouuuuld be ok now.

@zoontek zoontek merged commit 0ccfb96 into zoontek:master Oct 21, 2022
@zoontek
Copy link
Owner

zoontek commented Oct 21, 2022

Yep, it's perfect, thanks!

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.

2 participants