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

[expo-notifications][Android] Add default channel plugin prop, restore legacy icon and color #29491

Conversation

douglowder
Copy link
Contributor

Why

Based on customer feedback from expo-notifications@0.28.7, three adjustments are still needed:

  • Set both FCM legacy and FCMv1 metadata items in the Android manifest, so that icon and color work in both cases
  • Add a config plugin property, defaultChannel, to allow a developer to set the FCMv1 default channel in the manifest.
  • The Android lifecycle listeners should check to see if the intent extras have a notificationResponse object, and not map the intent bundle to create a duplicate response in JS.

See this comment by @mgscreativa and other comments in that issue.

How

  • Make the appropriate code changes in the plugin and in ExpoNotificationLifecycleListener.java

Test Plan

  • CI should pass
  • Test app patched with these changes should behave as expected

Checklist

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Jun 6, 2024
@expo-bot
Copy link
Collaborator

expo-bot commented Jun 6, 2024

The Pull Request introduced fingerprint changes against the base commit: 81cdcee

Fingerprint diff
[
  {
    "type": "file",
    "filePath": "../../packages/expo-notifications/plugin/build/withNotificationsAndroid.js",
    "reasons": [
      "expoConfigPlugins"
    ],
    "hash": "8852a43319643083dce105dd17a0d7fde531531a"
  },
  {
    "type": "dir",
    "filePath": "../../packages/expo-notifications/android",
    "reasons": [
      "expoAutolinkingAndroid"
    ],
    "hash": "047f6878c85fe0f7d43f9373eb0235f419acea8d"
  }
]

Generated by PR labeler 🤖

@douglowder douglowder marked this pull request as ready for review June 6, 2024 03:12
@douglowder douglowder requested a review from tsapeta as a code owner June 6, 2024 03:12
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Jun 6, 2024
@douglowder douglowder force-pushed the doug/eng-12457-clean-up-notifications-plugin-properties-and-code-for-icon branch from 9239a74 to 6844b7a Compare June 6, 2024 03:16
@douglowder
Copy link
Contributor Author

@amandeepmittal @Simek this will be cherry-picked to SDK 51, do I need to also make doc changes manually in docs/pages/versions/v51.0.0?

@amandeepmittal
Copy link
Member

... this will be cherry-picked to SDK 51, do I need to also make doc changes manually in docs/pages/versions/v51.0.0?

@doug, the data file (.json) for SDK 51 needs to be generated to apply changes to the SDK 51 API reference.

@douglowder
Copy link
Contributor Author

@amandeepmittal this doc change is not in the generated API .json file, it is in the manually written notifications.mdx page.

@douglowder douglowder merged commit 46ac36d into main Jun 7, 2024
@douglowder douglowder deleted the doug/eng-12457-clean-up-notifications-plugin-properties-and-code-for-icon branch June 7, 2024 18:46
@amandeepmittal
Copy link
Member

@amandeepmittal this doc change is not in the generated API .json file, it is in the manually written notifications.mdx page.

My bad, I assumed this was generated from the package source (as with other packages are 😅 ) and forgot this one isn't. I'll create a follow-up PR to apply the doc changes to SDK 51 reference page too, in a while.

douglowder added a commit that referenced this pull request Jun 7, 2024
…e legacy icon and color (#29491)

# Why

Based on customer feedback from `expo-notifications@0.28.7`, three
adjustments are still needed:

- Set both FCM legacy and FCMv1 metadata items in the Android manifest,
so that icon and color work in both cases
- Add a config plugin property, `defaultChannel`, to allow a developer
to set the FCMv1 default channel in the manifest.
- The Android lifecycle listeners should check to see if the intent
extras have a `notificationResponse` object, and not map the intent
bundle to create a duplicate response in JS.

See [this
comment](#28656 (comment))
by @mgscreativa and other comments in that issue.

# How

- Make the appropriate code changes in the plugin and in
`ExpoNotificationLifecycleListener.java`

# Test Plan

- CI should pass
- Test app patched with these changes should behave as expected

# Checklist

<!--
Please check the appropriate items below if they apply to your diff.
This is required for changes to Expo modules.
-->

- [ ] Documentation is up to date to reflect these changes (eg:
https://docs.expo.dev and README.md).
- [ ] Conforms with the [Documentation Writing Style
Guide](https://github.com/expo/expo/blob/main/guides/Expo%20Documentation%20Writing%20Style%20Guide.md)
- [ ] This diff will work correctly for `npx expo prebuild` & EAS Build
(eg: updated a module plugin).
@brentvatne brentvatne added the published Changes from the PR have been published to npm label Jun 10, 2024
amandeepmittal added a commit that referenced this pull request Jun 11, 2024
…ultChannel` (#29550)

# Why

<!--
Please describe the motivation for this PR, and link to relevant GitHub
issues, forums posts, or feature requests.
-->

Follow-up #29491

# How

<!--
How did you build this feature or fix this bug and why?
-->

Apply the same changes on `defaultChannel` Expo config property to SDK
51 after @douglowder mentioned that the changes will be cherry-picked
for SDK 51.

# Test Plan

<!--
Please describe how you tested this change and how a reviewer could
reproduce your test, especially if this PR does not include automated
tests! If possible, please also provide terminal output and/or
screenshots demonstrating your test/reproduction.
-->

Run docs locally.

## Preview of changes

![CleanShot 2024-06-08 at 00 32
18@2x](https://github.com/expo/expo/assets/10234615/dcddafb3-57cc-4854-ba8e-06759b039aa9)


# Checklist

<!--
Please check the appropriate items below if they apply to your diff.
This is required for changes to Expo modules.
-->

- [x] Documentation is up to date to reflect these changes (eg:
https://docs.expo.dev and README.md).
- [ ] Conforms with the [Documentation Writing Style
Guide](https://github.com/expo/expo/blob/main/guides/Expo%20Documentation%20Writing%20Style%20Guide.md)
- [ ] This diff will work correctly for `npx expo prebuild` & EAS Build
(eg: updated a module plugin).
marklawlor pushed a commit that referenced this pull request Jun 12, 2024
…ultChannel` (#29550)

# Why

<!--
Please describe the motivation for this PR, and link to relevant GitHub
issues, forums posts, or feature requests.
-->

Follow-up #29491

# How

<!--
How did you build this feature or fix this bug and why?
-->

Apply the same changes on `defaultChannel` Expo config property to SDK
51 after @douglowder mentioned that the changes will be cherry-picked
for SDK 51.

# Test Plan

<!--
Please describe how you tested this change and how a reviewer could
reproduce your test, especially if this PR does not include automated
tests! If possible, please also provide terminal output and/or
screenshots demonstrating your test/reproduction.
-->

Run docs locally.

## Preview of changes

![CleanShot 2024-06-08 at 00 32
18@2x](https://github.com/expo/expo/assets/10234615/dcddafb3-57cc-4854-ba8e-06759b039aa9)


# Checklist

<!--
Please check the appropriate items below if they apply to your diff.
This is required for changes to Expo modules.
-->

- [x] Documentation is up to date to reflect these changes (eg:
https://docs.expo.dev and README.md).
- [ ] Conforms with the [Documentation Writing Style
Guide](https://github.com/expo/expo/blob/main/guides/Expo%20Documentation%20Writing%20Style%20Guide.md)
- [ ] This diff will work correctly for `npx expo prebuild` & EAS Build
(eg: updated a module plugin).
douglowder added a commit that referenced this pull request Jun 12, 2024
…e legacy icon and color (#29491)

Based on customer feedback from `expo-notifications@0.28.7`, three
adjustments are still needed:

- Set both FCM legacy and FCMv1 metadata items in the Android manifest,
so that icon and color work in both cases
- Add a config plugin property, `defaultChannel`, to allow a developer
to set the FCMv1 default channel in the manifest.
- The Android lifecycle listeners should check to see if the intent
extras have a `notificationResponse` object, and not map the intent
bundle to create a duplicate response in JS.

See [this
comment](#28656 (comment))
by @mgscreativa and other comments in that issue.

- Make the appropriate code changes in the plugin and in
`ExpoNotificationLifecycleListener.java`

- CI should pass
- Test app patched with these changes should behave as expected

<!--
Please check the appropriate items below if they apply to your diff.
This is required for changes to Expo modules.
-->

- [ ] Documentation is up to date to reflect these changes (eg:
https://docs.expo.dev and README.md).
- [ ] Conforms with the [Documentation Writing Style
Guide](https://github.com/expo/expo/blob/main/guides/Expo%20Documentation%20Writing%20Style%20Guide.md)
- [ ] This diff will work correctly for `npx expo prebuild` & EAS Build
(eg: updated a module plugin).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: fingerprint changed bot: passed checks ExpoBot has nothing to complain about published Changes from the PR have been published to npm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants