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

[notifications] Add permission hooks from modules factory #13863

Merged
merged 5 commits into from
Aug 16, 2021

Conversation

byCedric
Copy link
Member

@byCedric byCedric commented Jul 30, 2021

Blocked by #13860

Why

Adds a permission hook from #13782 to expo-notifications.

Note, just like expo-location, this requires generics in the permission hook factory to inherit the customized permission type.

How

Exported permission hook and permission hook options type.

Test Plan

See example in docblock.

Checklist

  • Documentation is up to date to reflect these changes (eg: https://docs.expo.io and README.md).
  • This diff will work correctly for expo build (eg: updated @expo/xdl).
  • This diff will work correctly for expo prebuild & EAS Build (eg: updated a module plugin).

@expo-bot expo-bot added the bot: passed checks ExpoBot has nothing to complain about label Jul 30, 2021
Copy link
Collaborator

@expo-bot expo-bot left a comment

Choose a reason for hiding this comment

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

The review previously left here is no longer valid, jump to the latest one 👉 #13863 (review)

@expo-bot expo-bot added bot: suggestions ExpoBot has some suggestions and removed bot: passed checks ExpoBot has nothing to complain about labels Aug 3, 2021
@byCedric byCedric force-pushed the @bycedric/permissionhook/expo-notifications branch from 270e401 to 6fb90f3 Compare August 3, 2021 14:09
Copy link
Collaborator

@expo-bot expo-bot left a comment

Choose a reason for hiding this comment

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

The review previously left here is no longer valid, jump to the latest one 👉 #13863 (review)

@byCedric byCedric force-pushed the @bycedric/permissionhook/expo-notifications branch from 6fb90f3 to 7ce2518 Compare August 3, 2021 14:11
Copy link
Collaborator

@expo-bot expo-bot left a comment

Choose a reason for hiding this comment

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

The review previously left here is no longer valid, jump to the latest one 👉 #13863 (review)

@byCedric byCedric force-pushed the @bycedric/permissionhook/expo-notifications branch 2 times, most recently from ec81908 to 1f674d3 Compare August 3, 2021 14:14
Copy link
Collaborator

@expo-bot expo-bot left a comment

Choose a reason for hiding this comment

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

The review previously left here is no longer valid, jump to the latest one 👉 #13863 (review)

@byCedric byCedric force-pushed the @bycedric/permissionhook/expo-notifications branch from 1f674d3 to f9254a3 Compare August 3, 2021 14:15
Copy link
Collaborator

@expo-bot expo-bot left a comment

Choose a reason for hiding this comment

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

Hi there! 👋 I'm a bot whose goal is to ensure your contributions meet our guidelines.

I've found some issues in your pull request that should be addressed (click on them to expand) 👇

⚠️ Suggestions: Missing changelog entries


Your changes should be noted in the changelog. Read Updating Changelogs guide and consider (it's optional) adding an appropriate entry to the following changelogs:


Generated by ExpoBot 🤖 against f9254a3

@byCedric byCedric force-pushed the @bycedric/permissionhook/expo-notifications branch from f9254a3 to ff8dbff Compare August 16, 2021 13:48
@byCedric byCedric marked this pull request as ready for review August 16, 2021 13:48
@byCedric byCedric requested a review from cruzach as a code owner August 16, 2021 13:48
Copy link
Contributor

@cruzach cruzach left a comment

Choose a reason for hiding this comment

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

👍

packages/expo-notifications/CHANGELOG.md Outdated Show resolved Hide resolved
packages/expo-notifications/CHANGELOG.md Outdated Show resolved Hide resolved
@expo-bot
Copy link
Collaborator

Hi there! 👋 I'm a bot whose goal is to ensure your contributions meet our guidelines.

I've found some issues in your pull request that should be addressed (click on them for more details) 👇

⚠️ Suggestion: Missing changelog entries


Your changes should be noted in the changelog. Read Updating Changelogs guide and consider (it's optional) adding an appropriate entry to the following changelogs:


Generated by ExpoBot 🤖 against 1ec7d8e

@byCedric byCedric merged commit 5b1d074 into master Aug 16, 2021
@byCedric byCedric deleted the @bycedric/permissionhook/expo-notifications branch August 16, 2021 18:09
FelipeACP pushed a commit to FelipeACP/expo that referenced this pull request Sep 18, 2021
* [notifications] Add permission hooks from modules factory

* [notifications] Add changelog entry

* [modules-core] Add generic options to permission hook

* [notifications] Use proper permission requesters with options

* [notifications] Add the changelog line breaks back
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: suggestions ExpoBot has some suggestions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants